On 2025/07/11 22:02, Paolo Abeni wrote:
If the driver uses any of the extended features (i.e. above 64),
serialize the full features range (128 bits).
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>
---
v1 -> v2:
- uint128_t -> u64[2]
---
hw/virtio/virtio.c | 97 ++++++++++++++++++++++++++++++++++++++++------
1 file changed, 86 insertions(+), 11 deletions(-)
diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
index 82a285a31d..6a313313dd 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_array);
+}
+
+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_ARRAY(guest_features_array, VirtIODevice, 2),
We only need to save the second element so it can be reduced to:
VMSTATE_UINT64(guest_features_array[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,30 @@ 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;
+
+ virtio_features_andnot(tmp, val, vdev->host_features_array);
+ bad = !virtio_features_is_empty(tmp);
bitmap_andnot() returns a value representing if some bit in the
resulting bitmap is set. We can remove the virtio_features_is_empty()
call if virtio_features_andnot() does the same.
+
+ virtio_features_and(tmp, val, vdev->host_features_array);
- 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_array, tmp);
return bad ? -1 : 0;
}
typedef struct VirtioSetFeaturesNocheckData {
Coroutine *co;
VirtIODevice *vdev;
- uint64_t val;
+ uint64_t val[VIRTIO_FEATURES_DWORDS];
int ret;
} VirtioSetFeaturesNocheckData;
@@ -3094,12 +3120,41 @@ virtio_set_features_nocheck_maybe_co(VirtIODevice *vdev, uint64_t val)
VirtioSetFeaturesNocheckData data = {
.co = qemu_coroutine_self(),
.vdev = vdev,
- .val = val,
};
+ virtio_features_from_u64(data.val, val);
aio_bh_schedule_oneshot(qemu_get_current_aio_context(),
virtio_set_features_nocheck_bh, &data);
qemu_coroutine_yield();
return data.ret;
+ } else {
+ uint64_t features[VIRTIO_FEATURES_DWORDS];
+ virtio_features_from_u64(features, val);
+ return virtio_set_features_nocheck(vdev, features);
+ }
+}
+
+static void virtio_set_128bit_features_nocheck_bh(void *opaque)
"128bit" should be omitted for consistency with
virtio_set_features_nocheck() and for extensibility.
+{
+ VirtioSetFeaturesNocheckData *data = opaque;
+
+ data->ret = virtio_set_features_nocheck(data->vdev, data->val);
+ aio_co_wake(data->co);
+}
+
+static int coroutine_mixed_fn
+virtio_set_128bit_features_nocheck_maybe_co(VirtIODevice *vdev,
+ const uint64_t *val)
+{
+ if (qemu_in_coroutine()) {
+ VirtioSetFeaturesNocheckData data = {
+ .co = qemu_coroutine_self(),
+ .vdev = vdev,
+ };
+ virtio_features_copy(data.val, val);
+ aio_bh_schedule_oneshot(qemu_get_current_aio_context(),
+ virtio_set_128bit_features_nocheck_bh, &data);
+ qemu_coroutine_yield();
+ return data.ret;
} else {
return virtio_set_features_nocheck(vdev, val);
}
@@ -3107,6 +3162,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 +3178,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 +3202,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 +3229,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 +3313,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_array, features);
config_len = qemu_get_be32(f);
@@ -3333,7 +3392,23 @@ virtio_load(VirtIODevice *vdev, QEMUFile *f, int version_id)
vdev->device_endian = virtio_default_endian();
}
- if (virtio_64bit_features_needed(vdev)) {
+ /*
+ * Serialization needs constant size features array. Avoid
+ * silently breaking migration should the feature space increase
+ * even more in the (far away) future
Serialization is not done here and irrlevant.
+ */
+ QEMU_BUILD_BUG_ON(VIRTIO_FEATURES_DWORDS != 2);
+ if (virtio_128bit_features_needed(vdev)) {
There is no need to distinguish virtio_128bit_features_needed() and
virtio_64bit_features_needed() here.
For the 32-bit case, it will be simpler to have an array here and use
virtio_set_128bit_features_nocheck_maybe_co() instead of having
virtio_set_features_nocheck_maybe_co().
+ uint64_t *val = vdev->guest_features_array;
+
+ if (virtio_set_128bit_features_nocheck_maybe_co(vdev, val) < 0) {
+ error_report("Features 0x" VIRTIO_FEATURES_FMT " unsupported. "
+ "Allowed features: 0x" VIRTIO_FEATURES_FMT,
+ VIRTIO_FEATURES_PR(val),
+ VIRTIO_FEATURES_PR(vdev->host_features_array));
+ return -1;
+ }
+ } else if (virtio_64bit_features_needed(vdev)) {
/*
* Subsection load filled vdev->guest_features. Run them
* through virtio_set_features to sanity-check them against
