Extend the features configuration space to 128 bits, and allow the
common read/write operation to access all of it.
On migration, save the 128 bit version of the features only if the
upper bits are non zero; after load zero the upper bits if the extended
features were not loaded.
Note that we must clear the proxy-ed features on device reset, otherwise
a guest kernel not supporting extended features booted after an extended
features enabled one could end-up wrongly inheriting extended features.
Signed-off-by: Paolo Abeni <pabeni@redhat.com>
---
v1 -> v2:
- use separate VMStateDescription and pre/post load to avoid breaking
migration
- clear proxy features on device reset
---
hw/virtio/virtio-pci.c | 101 +++++++++++++++++++++++++++++----
include/hw/virtio/virtio-pci.h | 6 +-
2 files changed, 96 insertions(+), 11 deletions(-)
diff --git a/hw/virtio/virtio-pci.c b/hw/virtio/virtio-pci.c
index fba2372c93..dc5e7eaf81 100644
--- a/hw/virtio/virtio-pci.c
+++ b/hw/virtio/virtio-pci.c
@@ -108,6 +108,39 @@ static const VMStateDescription vmstate_virtio_pci_modern_queue_state = {
}
};
+static bool virtio_pci_modern_state_features128_needed(void *opaque)
+{
+ VirtIOPCIProxy *proxy = opaque;
+ uint32_t features = 0;
+ int i;
+
+ for (i = 2; i < ARRAY_SIZE(proxy->guest_features128); ++i) {
+ features |= proxy->guest_features128[i];
+ }
+ return !!features;
+}
+
+static int virtio_pci_modern_state_features128_post_load(void *opaque,
+ int version_id)
+{
+ VirtIOPCIProxy *proxy = opaque;
+
+ proxy->extended_features_loaded = true;
+ return 0;
+}
+
+static const VMStateDescription vmstate_virtio_pci_modern_state_features128 = {
+ .name = "virtio_pci/modern_state/features128",
+ .version_id = 1,
+ .minimum_version_id = 1,
+ .post_load = &virtio_pci_modern_state_features128_post_load,
+ .needed = &virtio_pci_modern_state_features128_needed,
+ .fields = (const VMStateField[]) {
+ VMSTATE_UINT32_ARRAY(guest_features128, VirtIOPCIProxy, 4),
+ VMSTATE_END_OF_LIST()
+ }
+};
+
static bool virtio_pci_modern_state_needed(void *opaque)
{
VirtIOPCIProxy *proxy = opaque;
@@ -115,10 +148,40 @@ static bool virtio_pci_modern_state_needed(void *opaque)
return virtio_pci_modern(proxy);
}
+static int virtio_pci_modern_state_pre_load(void *opaque)
+{
+ VirtIOPCIProxy *proxy = opaque;
+
+ proxy->extended_features_loaded = false;
+ return 0;
+}
+
+static int virtio_pci_modern_state_post_load(void *opaque, int version_id)
+{
+ VirtIOPCIProxy *proxy = opaque;
+ int i;
+
+ if (proxy->extended_features_loaded) {
+ return 0;
+ }
+
+ QEMU_BUILD_BUG_ON(offsetof(VirtIOPCIProxy, guest_features[0]) !=
+ offsetof(VirtIOPCIProxy, guest_features128[0]));
+ QEMU_BUILD_BUG_ON(offsetof(VirtIOPCIProxy, guest_features[1]) !=
+ offsetof(VirtIOPCIProxy, guest_features128[1]));
+
+ for (i = 2; i < ARRAY_SIZE(proxy->guest_features128); ++i) {
+ proxy->guest_features128[i] = 0;
+ }
+ return 0;
+}
+
static const VMStateDescription vmstate_virtio_pci_modern_state_sub = {
.name = "virtio_pci/modern_state",
.version_id = 1,
.minimum_version_id = 1,
+ .pre_load = &virtio_pci_modern_state_pre_load,
+ .post_load = &virtio_pci_modern_state_post_load,
.needed = &virtio_pci_modern_state_needed,
.fields = (const VMStateField[]) {
VMSTATE_UINT32(dfselect, VirtIOPCIProxy),
@@ -128,6 +191,10 @@ static const VMStateDescription vmstate_virtio_pci_modern_state_sub = {
vmstate_virtio_pci_modern_queue_state,
VirtIOPCIQueue),
VMSTATE_END_OF_LIST()
+ },
+ .subsections = (const VMStateDescription * const []) {
+ &vmstate_virtio_pci_modern_state_features128,
+ NULL
}
};
@@ -1493,19 +1560,22 @@ static uint64_t virtio_pci_common_read(void *opaque, hwaddr addr,
val = proxy->dfselect;
break;
case VIRTIO_PCI_COMMON_DF:
- if (proxy->dfselect <= 1) {
+ if (proxy->dfselect < VIRTIO_FEATURES_WORDS) {
VirtioDeviceClass *vdc = VIRTIO_DEVICE_GET_CLASS(vdev);
- val = (vdev->host_features & ~vdc->legacy_features) >>
- (32 * proxy->dfselect);
+ val = vdev->host_features_array[proxy->dfselect >> 1] >>
+ (32 * (proxy->dfselect & 1));
+ if (proxy->dfselect <= 1) {
+ val &= (~vdc->legacy_features) >> (32 * proxy->dfselect);
+ }
}
break;
case VIRTIO_PCI_COMMON_GFSELECT:
val = proxy->gfselect;
break;
case VIRTIO_PCI_COMMON_GF:
- if (proxy->gfselect < ARRAY_SIZE(proxy->guest_features)) {
- val = proxy->guest_features[proxy->gfselect];
+ if (proxy->gfselect < ARRAY_SIZE(proxy->guest_features128)) {
+ val = proxy->guest_features128[proxy->gfselect];
}
break;
case VIRTIO_PCI_COMMON_MSIX:
@@ -1587,11 +1657,18 @@ static void virtio_pci_common_write(void *opaque, hwaddr addr,
proxy->gfselect = val;
break;
case VIRTIO_PCI_COMMON_GF:
- if (proxy->gfselect < ARRAY_SIZE(proxy->guest_features)) {
- proxy->guest_features[proxy->gfselect] = val;
- virtio_set_features(vdev,
- (((uint64_t)proxy->guest_features[1]) << 32) |
- proxy->guest_features[0]);
+ if (proxy->gfselect < ARRAY_SIZE(proxy->guest_features128)) {
+ uint64_t features[VIRTIO_FEATURES_DWORDS];
+ int i;
+
+ proxy->guest_features128[proxy->gfselect] = val;
+ virtio_features_clear(features);
+ for (i = 0; i < ARRAY_SIZE(proxy->guest_features128); ++i) {
+ uint64_t cur = proxy->guest_features128[i];
+
+ features[i >> 1] |= cur << ((i & 1) * 32);
+ }
+ virtio_set_features_ex(vdev, features);
}
break;
case VIRTIO_PCI_COMMON_MSIX:
@@ -2310,6 +2387,10 @@ static void virtio_pci_reset(DeviceState *qdev)
virtio_bus_reset(bus);
msix_unuse_all_vectors(&proxy->pci_dev);
+ /* be sure to not carry over any feature across reset */
+ memset(proxy->guest_features128, 0, sizeof(uint32_t) *
+ ARRAY_SIZE(proxy->guest_features128));
+
for (i = 0; i < VIRTIO_QUEUE_MAX; i++) {
proxy->vqs[i].enabled = 0;
proxy->vqs[i].reset = 0;
diff --git a/include/hw/virtio/virtio-pci.h b/include/hw/virtio/virtio-pci.h
index eab5394898..1868e3b106 100644
--- a/include/hw/virtio/virtio-pci.h
+++ b/include/hw/virtio/virtio-pci.h
@@ -151,6 +151,7 @@ struct VirtIOPCIProxy {
uint32_t flags;
bool disable_modern;
bool ignore_backend_features;
+ bool extended_features_loaded;
OnOffAuto disable_legacy;
/* Transitional device id */
uint16_t trans_devid;
@@ -158,7 +159,10 @@ struct VirtIOPCIProxy {
uint32_t nvectors;
uint32_t dfselect;
uint32_t gfselect;
- uint32_t guest_features[2];
+ union {
+ uint32_t guest_features[2];
+ uint32_t guest_features128[4];
+ };
VirtIOPCIQueue vqs[VIRTIO_QUEUE_MAX];
VirtIOIRQFD *vector_irqfd;
--
2.50.0On 2025/07/11 22:02, Paolo Abeni wrote:
> Extend the features configuration space to 128 bits, and allow the
> common read/write operation to access all of it.
>
> On migration, save the 128 bit version of the features only if the
> upper bits are non zero; after load zero the upper bits if the extended
> features were not loaded.
>
> Note that we must clear the proxy-ed features on device reset, otherwise
> a guest kernel not supporting extended features booted after an extended
> features enabled one could end-up wrongly inheriting extended features.
>
> Signed-off-by: Paolo Abeni <pabeni@redhat.com>
> ---
> v1 -> v2:
> - use separate VMStateDescription and pre/post load to avoid breaking
> migration
> - clear proxy features on device reset
> ---
> hw/virtio/virtio-pci.c | 101 +++++++++++++++++++++++++++++----
> include/hw/virtio/virtio-pci.h | 6 +-
> 2 files changed, 96 insertions(+), 11 deletions(-)
>
> diff --git a/hw/virtio/virtio-pci.c b/hw/virtio/virtio-pci.c
> index fba2372c93..dc5e7eaf81 100644
> --- a/hw/virtio/virtio-pci.c
> +++ b/hw/virtio/virtio-pci.c
> @@ -108,6 +108,39 @@ static const VMStateDescription vmstate_virtio_pci_modern_queue_state = {
> }
> };
>
> +static bool virtio_pci_modern_state_features128_needed(void *opaque)
> +{
> + VirtIOPCIProxy *proxy = opaque;
> + uint32_t features = 0;
> + int i;
> +
> + for (i = 2; i < ARRAY_SIZE(proxy->guest_features128); ++i) {
> + features |= proxy->guest_features128[i];
> + }
> + return !!features;
"!!" is unnecessary; the implicit cast will clamp the value into true/false.
> +}
> +
> +static int virtio_pci_modern_state_features128_post_load(void *opaque,
> + int version_id)
> +{
> + VirtIOPCIProxy *proxy = opaque;
> +
> + proxy->extended_features_loaded = true;
> + return 0;
> +}
> +
> +static const VMStateDescription vmstate_virtio_pci_modern_state_features128 = {
> + .name = "virtio_pci/modern_state/features128",
> + .version_id = 1,
> + .minimum_version_id = 1,
> + .post_load = &virtio_pci_modern_state_features128_post_load,
> + .needed = &virtio_pci_modern_state_features128_needed,
> + .fields = (const VMStateField[]) {
> + VMSTATE_UINT32_ARRAY(guest_features128, VirtIOPCIProxy, 4),
> + VMSTATE_END_OF_LIST()
> + }
> +};
> +
> static bool virtio_pci_modern_state_needed(void *opaque)
> {
> VirtIOPCIProxy *proxy = opaque;
> @@ -115,10 +148,40 @@ static bool virtio_pci_modern_state_needed(void *opaque)
> return virtio_pci_modern(proxy);
> }
>
> +static int virtio_pci_modern_state_pre_load(void *opaque)
> +{
> + VirtIOPCIProxy *proxy = opaque;
> +
> + proxy->extended_features_loaded = false;
> + return 0;
> +}
> +
> +static int virtio_pci_modern_state_post_load(void *opaque, int version_id)
> +{
> + VirtIOPCIProxy *proxy = opaque;
> + int i;
> +
> + if (proxy->extended_features_loaded) {
> + return 0;
> + }
> +
> + QEMU_BUILD_BUG_ON(offsetof(VirtIOPCIProxy, guest_features[0]) !=
> + offsetof(VirtIOPCIProxy, guest_features128[0]));
> + QEMU_BUILD_BUG_ON(offsetof(VirtIOPCIProxy, guest_features[1]) !=
> + offsetof(VirtIOPCIProxy, guest_features128[1]));
> +
> + for (i = 2; i < ARRAY_SIZE(proxy->guest_features128); ++i) {
> + proxy->guest_features128[i] = 0;
> + }
> + return 0;
> +}
> +
You can expect the device is in the reset state when migrating so expect
guest_features128 is initialized as zero; there are already plenty of
code expecting the reset state.
> static const VMStateDescription vmstate_virtio_pci_modern_state_sub = {
> .name = "virtio_pci/modern_state",
> .version_id = 1,
> .minimum_version_id = 1,
> + .pre_load = &virtio_pci_modern_state_pre_load,
> + .post_load = &virtio_pci_modern_state_post_load,
> .needed = &virtio_pci_modern_state_needed,
> .fields = (const VMStateField[]) {
> VMSTATE_UINT32(dfselect, VirtIOPCIProxy),
> @@ -128,6 +191,10 @@ static const VMStateDescription vmstate_virtio_pci_modern_state_sub = {
> vmstate_virtio_pci_modern_queue_state,
> VirtIOPCIQueue),
> VMSTATE_END_OF_LIST()
> + },
> + .subsections = (const VMStateDescription * const []) {
> + &vmstate_virtio_pci_modern_state_features128,
> + NULL
> }
> };
>
> @@ -1493,19 +1560,22 @@ static uint64_t virtio_pci_common_read(void *opaque, hwaddr addr,
> val = proxy->dfselect;
> break;
> case VIRTIO_PCI_COMMON_DF:
> - if (proxy->dfselect <= 1) {
> + if (proxy->dfselect < VIRTIO_FEATURES_WORDS) {
> VirtioDeviceClass *vdc = VIRTIO_DEVICE_GET_CLASS(vdev);
>
> - val = (vdev->host_features & ~vdc->legacy_features) >>
> - (32 * proxy->dfselect);
> + val = vdev->host_features_array[proxy->dfselect >> 1] >>
> + (32 * (proxy->dfselect & 1));
> + if (proxy->dfselect <= 1) {
> + val &= (~vdc->legacy_features) >> (32 * proxy->dfselect);
> + }
> }
> break;
> case VIRTIO_PCI_COMMON_GFSELECT:
> val = proxy->gfselect;
> break;
> case VIRTIO_PCI_COMMON_GF:
> - if (proxy->gfselect < ARRAY_SIZE(proxy->guest_features)) {
> - val = proxy->guest_features[proxy->gfselect];
> + if (proxy->gfselect < ARRAY_SIZE(proxy->guest_features128)) {
> + val = proxy->guest_features128[proxy->gfselect];
> }
> break;
> case VIRTIO_PCI_COMMON_MSIX:
> @@ -1587,11 +1657,18 @@ static void virtio_pci_common_write(void *opaque, hwaddr addr,
> proxy->gfselect = val;
> break;
> case VIRTIO_PCI_COMMON_GF:
> - if (proxy->gfselect < ARRAY_SIZE(proxy->guest_features)) {
> - proxy->guest_features[proxy->gfselect] = val;
> - virtio_set_features(vdev,
> - (((uint64_t)proxy->guest_features[1]) << 32) |
> - proxy->guest_features[0]);
> + if (proxy->gfselect < ARRAY_SIZE(proxy->guest_features128)) {
> + uint64_t features[VIRTIO_FEATURES_DWORDS];
> + int i;
> +
> + proxy->guest_features128[proxy->gfselect] = val;
> + virtio_features_clear(features);
> + for (i = 0; i < ARRAY_SIZE(proxy->guest_features128); ++i) {
> + uint64_t cur = proxy->guest_features128[i];
> +
> + features[i >> 1] |= cur << ((i & 1) * 32);
> + }
> + virtio_set_features_ex(vdev, features);
> }
> break;
> case VIRTIO_PCI_COMMON_MSIX:
> @@ -2310,6 +2387,10 @@ static void virtio_pci_reset(DeviceState *qdev)
> virtio_bus_reset(bus);
> msix_unuse_all_vectors(&proxy->pci_dev);
>
> + /* be sure to not carry over any feature across reset */
It's obvious so I don't think the comment makes difference.
> + memset(proxy->guest_features128, 0, sizeof(uint32_t) *
> + ARRAY_SIZE(proxy->guest_features128));
Simpler:
memset(proxy->guest_features128, 0, sizeof(proxy->guest_features128);
> +
> for (i = 0; i < VIRTIO_QUEUE_MAX; i++) {
> proxy->vqs[i].enabled = 0;
> proxy->vqs[i].reset = 0;
> diff --git a/include/hw/virtio/virtio-pci.h b/include/hw/virtio/virtio-pci.h
> index eab5394898..1868e3b106 100644
> --- a/include/hw/virtio/virtio-pci.h
> +++ b/include/hw/virtio/virtio-pci.h
> @@ -151,6 +151,7 @@ struct VirtIOPCIProxy {
> uint32_t flags;
> bool disable_modern;
> bool ignore_backend_features;
> + bool extended_features_loaded;
> OnOffAuto disable_legacy;
> /* Transitional device id */
> uint16_t trans_devid;
> @@ -158,7 +159,10 @@ struct VirtIOPCIProxy {
> uint32_t nvectors;
> uint32_t dfselect;
> uint32_t gfselect;
> - uint32_t guest_features[2];
> + union {
> + uint32_t guest_features[2];
> + uint32_t guest_features128[4];
> + };
I don't see anything preventing you from directly extending guest_features.
> VirtIOPCIQueue vqs[VIRTIO_QUEUE_MAX];
>
> VirtIOIRQFD *vector_irqfd;
On 7/15/25 9:42 AM, Akihiko Odaki wrote:
> On 2025/07/11 22:02, Paolo Abeni wrote:
>> @@ -158,7 +159,10 @@ struct VirtIOPCIProxy {
>> uint32_t nvectors;
>> uint32_t dfselect;
>> uint32_t gfselect;
>> - uint32_t guest_features[2];
>> + union {
>> + uint32_t guest_features[2];
>> + uint32_t guest_features128[4];
>> + };
>
> I don't see anything preventing you from directly extending guest_features.
Uhmm... I have a vague memory of some interim revisions doing that and
failing miserably (but I have no log of the actual details). I'll try to
have another shot at it.
Thanks,
Paolo
On 7/15/25 6:21 PM, Paolo Abeni wrote:
> On 7/15/25 9:42 AM, Akihiko Odaki wrote:
>> On 2025/07/11 22:02, Paolo Abeni wrote:
>>> @@ -158,7 +159,10 @@ struct VirtIOPCIProxy {
>>> uint32_t nvectors;
>>> uint32_t dfselect;
>>> uint32_t gfselect;
>>> - uint32_t guest_features[2];
>>> + union {
>>> + uint32_t guest_features[2];
>>> + uint32_t guest_features128[4];
>>> + };
>>
>> I don't see anything preventing you from directly extending guest_features.
>
> Uhmm... I have a vague memory of some interim revisions doing that and
> failing miserably (but I have no log of the actual details). I'll try to
> have another shot at it.
The VMSTATE_ARRAY() macro has explicit checks on the specified array
matching exactly the specified array size. Using a single:
uint32_t guest_features[4];
variable, this statement
VMSTATE_UINT32_ARRAY(guest_features, VirtIOPCIProxy, 2),
causes the following build error:
--
include/migration/vmstate.h:259:48: error: invalid operands to binary -
(have ‘uint32_t (*)[2]’ {aka ‘unsigned int (*)[2]’} and ‘uint32_t
(*)[4]’ {aka ‘unsigned int (*)[4]’})
259 | #define type_check_array(t1,t2,n) ((t1(*)[n])0 - (t2*)0)
| ^
include/migration/vmstate.h:282:6: note: in expansion of macro
‘type_check_array’
282 | type_check_array(_type, typeof_field(_state, _field), _num))
| ^~~~~~~~~~~~~~~~
include/migration/vmstate.h:373:19: note: in expansion of macro
‘vmstate_offset_array’
373 | .offset = vmstate_offset_array(_state, _field, _type,
_num), \
| ^~~~~~~~~~~~~~~~~~~~
include/migration/vmstate.h:1090:5: note: in expansion of macro
‘VMSTATE_ARRAY’
1090 | VMSTATE_ARRAY(_f, _s, _n, _v, vmstate_info_uint32, uint32_t)
| ^~~~~~~~~~~~~
include/migration/vmstate.h:1096:5: note: in expansion of macro
‘VMSTATE_UINT32_ARRAY_V’
1096 | VMSTATE_UINT32_ARRAY_V(_f, _s, _n, 0)
| ^~~~~~~~~~~~~~~~~~~~~~
../hw/virtio/virtio-pci.c:168:9: note: in expansion of macro
‘VMSTATE_UINT32_ARRAY’
168 | VMSTATE_UINT32_ARRAY(guest_features, VirtIOPCIProxy, 2),
| ^~~~~~~~~~~~~~~~~~~~
--
I'll keep the union here.
Thanks,
Paolo
On 2025/07/16 18:14, Paolo Abeni wrote:
> On 7/15/25 6:21 PM, Paolo Abeni wrote:
>> On 7/15/25 9:42 AM, Akihiko Odaki wrote:
>>> On 2025/07/11 22:02, Paolo Abeni wrote:
>>>> @@ -158,7 +159,10 @@ struct VirtIOPCIProxy {
>>>> uint32_t nvectors;
>>>> uint32_t dfselect;
>>>> uint32_t gfselect;
>>>> - uint32_t guest_features[2];
>>>> + union {
>>>> + uint32_t guest_features[2];
>>>> + uint32_t guest_features128[4];
>>>> + };
>>>
>>> I don't see anything preventing you from directly extending guest_features.
>>
>> Uhmm... I have a vague memory of some interim revisions doing that and
>> failing miserably (but I have no log of the actual details). I'll try to
>> have another shot at it.
>
> The VMSTATE_ARRAY() macro has explicit checks on the specified array
> matching exactly the specified array size. Using a single:
>
> uint32_t guest_features[4];
>
> variable, this statement
>
> VMSTATE_UINT32_ARRAY(guest_features, VirtIOPCIProxy, 2),
>
> causes the following build error:
>
> --
> include/migration/vmstate.h:259:48: error: invalid operands to binary -
> (have ‘uint32_t (*)[2]’ {aka ‘unsigned int (*)[2]’} and ‘uint32_t
> (*)[4]’ {aka ‘unsigned int (*)[4]’})
> 259 | #define type_check_array(t1,t2,n) ((t1(*)[n])0 - (t2*)0)
> | ^
> include/migration/vmstate.h:282:6: note: in expansion of macro
> ‘type_check_array’
> 282 | type_check_array(_type, typeof_field(_state, _field), _num))
> | ^~~~~~~~~~~~~~~~
> include/migration/vmstate.h:373:19: note: in expansion of macro
> ‘vmstate_offset_array’
> 373 | .offset = vmstate_offset_array(_state, _field, _type,
> _num), \
> | ^~~~~~~~~~~~~~~~~~~~
> include/migration/vmstate.h:1090:5: note: in expansion of macro
> ‘VMSTATE_ARRAY’
> 1090 | VMSTATE_ARRAY(_f, _s, _n, _v, vmstate_info_uint32, uint32_t)
> | ^~~~~~~~~~~~~
> include/migration/vmstate.h:1096:5: note: in expansion of macro
> ‘VMSTATE_UINT32_ARRAY_V’
> 1096 | VMSTATE_UINT32_ARRAY_V(_f, _s, _n, 0)
> | ^~~~~~~~~~~~~~~~~~~~~~
> ../hw/virtio/virtio-pci.c:168:9: note: in expansion of macro
> ‘VMSTATE_UINT32_ARRAY’
> 168 | VMSTATE_UINT32_ARRAY(guest_features, VirtIOPCIProxy, 2),
> | ^~~~~~~~~~~~~~~~~~~~
> --
>
> I'll keep the union here.
I think you can use VMSTATE_UINT32_SUB_ARRAY().
Regards,
Akihiko Odaki
© 2016 - 2025 Red Hat, Inc.