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 <pabeni@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),
+ 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);
+
+ 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)
+{
+ 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
+ */
+ QEMU_BUILD_BUG_ON(VIRTIO_FEATURES_DWORDS != 2);
+ if (virtio_128bit_features_needed(vdev)) {
+ 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
--
2.50.0On 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 <pabeni@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
On 7/15/25 9:24 AM, Akihiko Odaki wrote:
> On 2025/07/11 22:02, Paolo Abeni wrote:
>> + */
>> + 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.
Double checking I'm reading the above correctly. Are you suggesting to
replace this chunk with something alike:
if (virtio_64bit_features_needed(vdev)) {
/* The 64 highest bit has been cleared by the previous
* virtio_features_from_u64() and ev.
* initialized as needed when loading
* "virtio/128bit_features"*/
uint64_t *val = vdev->guest_features_array;
if (virtio_set_128bit_features_nocheck_maybe_co(vdev, val) < 0)
// ...
> 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().
Again double checking I'm parsing the above correctly. You are
suggesting to dismiss the virtio_set_features_nocheck_maybe_co() helper
entirely and use virtio_set_128bit_features_nocheck_maybe_co() even when
only 32bit features are loaded. Am I correct?
Thanks,
Paolo
On 2025/07/16 0:40, Paolo Abeni wrote:
> On 7/15/25 9:24 AM, Akihiko Odaki wrote:
>> On 2025/07/11 22:02, Paolo Abeni wrote:
>>> + */
>>> + 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.
>
> Double checking I'm reading the above correctly. Are you suggesting to
> replace this chunk with something alike:
>
> if (virtio_64bit_features_needed(vdev)) {
This condition is not right as virtio_64bit_features_needed() doesn't
return true when the some of bits [64, 128) is set while bits [32, 64)
are cleared. I see two options to fix:
- Check: virtio_64bit_features_needed(vdev) ||
virtio_128bit_features_needed(vdev)
- Ensure that virtio_64bit_features_needed(vdev) returns true when a bit
more significant than bit 31 is set.
> /* The 64 highest bit has been cleared by the previous
> * virtio_features_from_u64() and ev.
> * initialized as needed when loading
> * "virtio/128bit_features"*/
> uint64_t *val = vdev->guest_features_array;
>
> if (virtio_set_128bit_features_nocheck_maybe_co(vdev, val) < 0)
> // ...> >> 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().
>
> Again double checking I'm parsing the above correctly. You are
> suggesting to dismiss the virtio_set_features_nocheck_maybe_co() helper
> entirely and use virtio_set_128bit_features_nocheck_maybe_co() even when
> only 32bit features are loaded. Am I correct?
Yes, but now I found it is unnecessary to special-case even the 32-bit case.
Commit 019a3edbb25f ("virtio: make features 64bit wide") had to add a
conditional to distinguish the 64-bit and 32-bit cases because
vdev->guest_features was not set before executing this part of code.
However, commit 62cee1a28aad ("virtio: set low features early on load")
later added preceding code to set vdev->guest_features. In summary, this
part of code can be simply replaced with:
if (virtio_set_128bit_features_nocheck_maybe_co(vdev,
vdev->guest_features_array) < 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;
}
There is no need of virtio_64bit_features_needed(vdev) or
virtio_128bit_features_needed(vdev) at all.
I have another finding by the way; there are three phrases that refers
to the new extension: array (e.g., guest_features_array), _ex (e.g.,
virtio_add_feature_ex), 128bit (e.g., virtio_128bit_features_needed).
It makes sense to make "128bit" an exception in the migration code
because the migration format is fixed and will require e.g., "192bit"
for a future extension. But two suffixes, _ex and _array, can be unified.
Regards,
Akihiko Odaki
© 2016 - 2025 Red Hat, Inc.