pcie migration fixes

While testing q35 live migration, I found that the migration would abort with the following error: "Unknown savevm section type 76".

The error is due to this check failing in 'vmstate_load_state()':

    while(field->name) {
        if ((field->field_exists &&
             field->field_exists(opaque, version_id)) ||
            (!field->field_exists &&
             field->version_id <= version_id)) {

The VMSTATE_PCIE_DEVICE() currently has a 'version_id' set to 2. However, 'version_id' in the above check is 1. And thus we fail to load the pcie device field. Further the code returns to 'qemu_loadvm_state()' which produces the error that I saw.

I'm proposing to fix this by simply dropping the 'version_id' field from VMSTATE_PCIE_DEVICE(). VMSTATE_PCI_DEVICE() defines no such field and further the vmstate_pcie_device that VMSTATE_PCI_DEVICE() refers to is already versioned. Thus, any versioning issues could be detected at the vmsd level.

Taking a step back, I think that the 'field->version_id' should be compared against a saved version number for the field not the 'version_id'. Futhermore, once vmstate_load_state() is called recursively on another vmsd, the check of:

    if (version_id > vmsd->version_id) {
        return -EINVAL;
    }

Will never fail since version_id is always equal to vmsd->version_id. So I'm wondering why we aren't storing the vmsd version id of the source in the migration stream?

This patch also renames the 'name' field of vmstate_pcie_device from:
PCIDevice -> PCIEDevice to differentiate it from vmstate_pci_device.

Signed-off-by Jason Baron <[email protected]>

---
 hw/pci.c  |    2 +-
 hw/pcie.h |    1 -
 2 files changed, 1 insertions(+), 2 deletions(-)

hw/pci.c +1 -1

index 3727afa..5386a4f 100644
@@ -439,7 +439,7 @@ const VMStateDescription vmstate_pci_device = {
 };
 
 const VMStateDescription vmstate_pcie_device = {
-    .name = "PCIDevice",
+    .name = "PCIEDevice",
     .version_id = 2,
     .minimum_version_id = 1,
     .minimum_version_id_old = 1,

hw/pcie.h +0 -1

index b8ab0c7..4889194 100644
@@ -133,7 +133,6 @@ extern const VMStateDescription vmstate_pcie_device;
 
 #define VMSTATE_PCIE_DEVICE(_field, _state) {                        \
     .name       = (stringify(_field)),                               \
-    .version_id = 2,                                                 \
     .size       = sizeof(PCIDevice),                                 \
     .vmsd       = &vmstate_pcie_device,                              \
     .flags      = VMS_STRUCT,                                        \

Signature · 1 line

1.7.1

Quoted 33 lines · Jason Baron

While testing q35 live migration, I found that the migration would abort with the following error: "Unknown savevm section type 76".

The error is due to this check failing in 'vmstate_load_state()':

    while(field->name) {
        if ((field->field_exists &&
             field->field_exists(opaque, version_id)) ||
            (!field->field_exists &&
             field->version_id <= version_id)) {

The VMSTATE_PCIE_DEVICE() currently has a 'version_id' set to 2. However, 'version_id' in the above check is 1. And thus we fail to load the pcie device field. Further the code returns to 'qemu_loadvm_state()' which produces the error that I saw.

I'm proposing to fix this by simply dropping the 'version_id' field from VMSTATE_PCIE_DEVICE(). VMSTATE_PCI_DEVICE() defines no such field and further the vmstate_pcie_device that VMSTATE_PCI_DEVICE() refers to is already versioned. Thus, any versioning issues could be detected at the vmsd level.

Taking a step back, I think that the 'field->version_id' should be compared against a saved version number for the field not the 'version_id'. Futhermore, once vmstate_load_state() is called recursively on another vmsd, the check of:

    if (version_id > vmsd->version_id) {
        return -EINVAL;
    }

Will never fail since version_id is always equal to vmsd->version_id. So I'm wondering why we aren't storing the vmsd version id of the source in the migration stream?

Quoted 4 lines

This patch also renames the 'name' field of vmstate_pcie_device from:
PCIDevice -> PCIEDevice to differentiate it from vmstate_pci_device.

Signed-off-by Jason Baron <[email protected]>

I have this queued to apply already - this is just a repost or did I miss something?

Quoted 37 lines · Michael S. Tsirkin

Quoted 36 lines · Jason Baron

While testing q35 live migration, I found that the migration would abort with the following error: "Unknown savevm section type 76".

The error is due to this check failing in 'vmstate_load_state()':

    while(field->name) {
        if ((field->field_exists &&
             field->field_exists(opaque, version_id)) ||
            (!field->field_exists &&
             field->version_id <= version_id)) {

The VMSTATE_PCIE_DEVICE() currently has a 'version_id' set to 2. However, 'version_id' in the above check is 1. And thus we fail to load the pcie device field. Further the code returns to 'qemu_loadvm_state()' which produces the error that I saw.

I'm proposing to fix this by simply dropping the 'version_id' field from VMSTATE_PCIE_DEVICE(). VMSTATE_PCI_DEVICE() defines no such field and further the vmstate_pcie_device that VMSTATE_PCI_DEVICE() refers to is already versioned. Thus, any versioning issues could be detected at the vmsd level.

Taking a step back, I think that the 'field->version_id' should be compared against a saved version number for the field not the 'version_id'. Futhermore, once vmstate_load_state() is called recursively on another vmsd, the check of:

    if (version_id > vmsd->version_id) {
        return -EINVAL;
    }

Will never fail since version_id is always equal to vmsd->version_id. So I'm wondering why we aren't storing the vmsd version id of the source in the migration stream?

This patch also renames the 'name' field of vmstate_pcie_device from:
PCIDevice -> PCIEDevice to differentiate it from vmstate_pci_device.

Quoted 5 lines

Quoted 1 line

Signed-off-by Jason Baron <[email protected]>

I have this queued to apply already - this is just a repost or did I miss something?

just a repost...but now we have Juan's ack :)

While testing q35 live migration, I found that the migration would abort with the following error: "Unknown savevm section type 76".

The error is due to this check failing in 'vmstate_load_state()':

    while(field->name) {
        if ((field->field_exists &&
             field->field_exists(opaque, version_id)) ||
            (!field->field_exists &&
             field->version_id <= version_id)) {

The VMSTATE_PCIE_DEVICE() currently has a 'version_id' set to 2. However, 'version_id' in the above check is 1. And thus we fail to load the pcie device field. Further the code returns to 'qemu_loadvm_state()' which produces the error that I saw.

I'm proposing to fix this by simply dropping the 'version_id' field from VMSTATE_PCIE_DEVICE(). VMSTATE_PCI_DEVICE() defines no such field and further the vmstate_pcie_device that VMSTATE_PCI_DEVICE() refers to is already versioned. Thus, any versioning issues could be detected at the vmsd level.

Taking a step back, I think that the 'field->version_id' should be compared against a saved version number for the field not the 'version_id'. Futhermore, once vmstate_load_state() is called recursively on another vmsd, the check of:

    if (version_id > vmsd->version_id) {
        return -EINVAL;
    }

Will never fail since version_id is always equal to vmsd->version_id. So I'm wondering why we aren't storing the vmsd version id of the source in the migration stream?

This patch also renames the 'name' field of vmstate_pcie_device from:
PCIDevice -> PCIEDevice to differentiate it from vmstate_pci_device.

Signed-off-by Jason Baron <[email protected]>

The Advanced Error Interrupt Message Number (bits 31:27 of the Root Error Status Register) is updated when the number of msi messages assigned to a device changes. Migration of windows 7 on q35 chipset failed because the check in get_pci_config_device() fails due to wmask being set on these bits.

Its valid
to update these bits and we must restore this state across migration.

Signed-off-by Jason Baron <[email protected]>

---
 hw/pcie_aer.c |    6 ++++++
 1 files changed, 6 insertions(+), 0 deletions(-)

hw/pcie_aer.c +0 -0

index 3b6981c..6edcd79 100644

hw/pcie_aer.c +0 -0

 }
 
 

void pcie_aer_root_reset(PCIDevice *dev)

Signature · 1 line

1.7.1

Some minor nits below. If you dont get to it I will tweak this patch when I apply it early next week.

Quoted 4 lines · Jason Baron

The Advanced Error Interrupt Message Number (bits 31:27 of the Root Error Status Register) is updated when the number of msi messages assigned to a device changes. Migration of windows 7 on q35 chipset failed because the check in get_pci_config_device() fails due to wmask being set on these bits.

I think you actually mean 'not being set on these bits'?

Quoted 8 lines

Its valid
to update these bits and we must restore this state across migration.

Signed-off-by Jason Baron <[email protected]>

---
 hw/pcie_aer.c |    6 ++++++
 1 files changed, 6 insertions(+), 0 deletions(-)

hw/pcie_aer.c void pcie_aer_root_init(PCIDevice *dev) · line 741

Quoted 9 lines

hw/pcie_aer.c +1 -0

index 3b6981c..6edcd79 100644
@@ -738,6 +738,12 @@ void pcie_aer_root_init(PCIDevice *dev)
                  PCI_ERR_ROOT_CMD_EN_MASK);
     pci_set_long(dev->w1cmask + pos + PCI_ERR_ROOT_STATUS,
                  PCI_ERR_ROOT_STATUS_REPORT_MASK);
+    /* Bits 31:27 - Advanced Error Interrupt Message Number

This line is better moved to near definition of PCI_ERR_ROOT_IRQ. Then here we can say 'PCI_ERR_ROOT_IRQ is RO but devices change it using a device-specific method.'

hw/pcie_aer.c void pcie_aer_root_init(PCIDevice *dev) · line 746

Quoted 6 lines

hw/pcie_aer.c +5 -0

+     * These bits are updated when the number of MSI messages changes.
+     * By clearing the cmask, pcie devices can be migrated.
+     */
+    pci_set_long(dev->cmask + pos + PCI_ERR_ROOT_STATUS,
+                 (1 << PCI_ERR_ROOT_IRQ_SHIFT) - 1);

~PCI_ERR_ROOT_IRQ would be clearer I think.

Quoted 11 lines · Michael S. Tsirkin

Some minor nits below. If you dont get to it I will tweak this patch when I apply it early next week.

Quoted 4 lines · Jason Baron

The Advanced Error Interrupt Message Number (bits 31:27 of the Root Error Status Register) is updated when the number of msi messages assigned to a device changes. Migration of windows 7 on q35 chipset failed because the check in get_pci_config_device() fails due to wmask being set on these bits.

I think you actually mean 'not being set on these bits'?

No, the check is:

static int get_pci_config_device(QEMUFile *f, void *pv, size_t size)

{
    PCIDevice *s = container_of(pv, PCIDevice, config);
    uint8_t *config;
    int i;

    assert(size == pci_config_size(s));
    config = g_malloc(size);

    qemu_get_buffer(f, config, size);
    for (i = 0; i < size; ++i) {
        if ((config[i] ^ s->config[i]) &
            s->cmask[i] & ~s->wmask[i] & ~s->w1cmask[i]) {
            g_free(config);
            return -EINVAL;
        }
    }

.....

So because cmask is set and these bits differ in config space (due to them being updated before migration), the migration aborts.

Quoted 9 lines

Quoted 9 lines

Its valid
to update these bits and we must restore this state across migration.

Signed-off-by Jason Baron <[email protected]>

---
 hw/pcie_aer.c |    6 ++++++
 1 files changed, 6 insertions(+), 0 deletions(-)

Quoted 13 lines

hw/pcie_aer.c void pcie_aer_root_init(PCIDevice *dev) · line 741

Quoted 9 lines

hw/pcie_aer.c +1 -0

index 3b6981c..6edcd79 100644
@@ -738,6 +738,12 @@ void pcie_aer_root_init(PCIDevice *dev)
                  PCI_ERR_ROOT_CMD_EN_MASK);
     pci_set_long(dev->w1cmask + pos + PCI_ERR_ROOT_STATUS,
                  PCI_ERR_ROOT_STATUS_REPORT_MASK);
+    /* Bits 31:27 - Advanced Error Interrupt Message Number

This line is better moved to near definition of PCI_ERR_ROOT_IRQ. Then here we can say 'PCI_ERR_ROOT_IRQ is RO but devices change it using a device-specific method.'

ok.

Quoted 9 lines

hw/pcie_aer.c void pcie_aer_root_init(PCIDevice *dev) · line 746

Quoted 6 lines

hw/pcie_aer.c +5 -0

+     * These bits are updated when the number of MSI messages changes.
+     * By clearing the cmask, pcie devices can be migrated.
+     */
+    pci_set_long(dev->cmask + pos + PCI_ERR_ROOT_STATUS,
+                 (1 << PCI_ERR_ROOT_IRQ_SHIFT) - 1);

~PCI_ERR_ROOT_IRQ would be clearer I think.

agreed.

I will re-spin and post a v2.

Thanks,

-Jason

Quoted 34 lines · Jason Baron

Quoted 11 lines · Michael S. Tsirkin

Some minor nits below. If you dont get to it I will tweak this patch when I apply it early next week.

Quoted 4 lines · Jason Baron

The Advanced Error Interrupt Message Number (bits 31:27 of the Root Error Status Register) is updated when the number of msi messages assigned to a device changes. Migration of windows 7 on q35 chipset failed because the check in get_pci_config_device() fails due to wmask being set on these bits.

I think you actually mean 'not being set on these bits'?

No, the check is:

static int get_pci_config_device(QEMUFile *f, void *pv, size_t size)

{
    PCIDevice *s = container_of(pv, PCIDevice, config);
    uint8_t *config;
    int i;

    assert(size == pci_config_size(s));
    config = g_malloc(size);

    qemu_get_buffer(f, config, size);
    for (i = 0; i < size; ++i) {
        if ((config[i] ^ s->config[i]) &
            s->cmask[i] & ~s->wmask[i] & ~s->w1cmask[i]) {
            g_free(config);
            return -EINVAL;
        }
    }

.....

Quoted 2 lines

So because cmask is set and these bits differ in config space (due to them being updated before migration), the migration aborts.

Ah so you mean 'cmask being set' - not wmask as you wrote.

Quoted 35 lines · Michael S. Tsirkin

Quoted 34 lines · Jason Baron

Quoted 11 lines · Michael S. Tsirkin

Some minor nits below. If you dont get to it I will tweak this patch when I apply it early next week.

Quoted 4 lines · Jason Baron

The Advanced Error Interrupt Message Number (bits 31:27 of the Root Error Status Register) is updated when the number of msi messages assigned to a device changes. Migration of windows 7 on q35 chipset failed because the check in get_pci_config_device() fails due to wmask being set on these bits.

I think you actually mean 'not being set on these bits'?

No, the check is:

static int get_pci_config_device(QEMUFile *f, void *pv, size_t size)

{
    PCIDevice *s = container_of(pv, PCIDevice, config);
    uint8_t *config;
    int i;

    assert(size == pci_config_size(s));
    config = g_malloc(size);

    qemu_get_buffer(f, config, size);
    for (i = 0; i < size; ++i) {
        if ((config[i] ^ s->config[i]) &
            s->cmask[i] & ~s->wmask[i] & ~s->w1cmask[i]) {
            g_free(config);
            return -EINVAL;
        }
    }

.....

Quoted 5 lines

Quoted 2 lines

So because cmask is set and these bits differ in config space (due to them being updated before migration), the migration aborts.

Ah so you mean 'cmask being set' - not wmask as you wrote.

Sorry - my mistake, yes I meant 'cmask'. I will fix the changelog in the re-post.

Quoted 42 lines · Jason Baron

Quoted 40 lines · Michael S. Tsirkin

Quoted 32 lines · Jason Baron

Quoted 11 lines · Michael S. Tsirkin

Some minor nits below. If you dont get to it I will tweak this patch when I apply it early next week.

Quoted 4 lines · Jason Baron

The Advanced Error Interrupt Message Number (bits 31:27 of the Root Error Status Register) is updated when the number of msi messages assigned to a device changes. Migration of windows 7 on q35 chipset failed because the check in get_pci_config_device() fails due to wmask being set on these bits.

I think you actually mean 'not being set on these bits'?

No, the check is:

static int get_pci_config_device(QEMUFile *f, void *pv, size_t size)

{
    PCIDevice *s = container_of(pv, PCIDevice, config);
    uint8_t *config;
    int i;

    assert(size == pci_config_size(s));
    config = g_malloc(size);

    qemu_get_buffer(f, config, size);
    for (i = 0; i < size; ++i) {
        if ((config[i] ^ s->config[i]) &
            s->cmask[i] & ~s->wmask[i] & ~s->w1cmask[i]) {
            g_free(config);
            return -EINVAL;
        }
    }

Quoted 4 lines

.....

So because cmask is set and these bits differ in config space (due to them being updated before migration), the migration aborts.

Ah so you mean 'cmask being set' - not wmask as you wrote.

Quoted 3 lines

Sorry - my mistake, yes I meant 'cmask'. I will fix the changelog in the re-post.

Ok, here's a v2:

---

pcie_aer: clear cmask for Advanced Error Interrupt Message Number

The Advanced Error Interrupt Message Number (bits 31:27 of the Root Error Status Register) is updated when the number of msi messages assigned to a device changes. Migration of windows 7 on q35 chipset failed because the check in get_pci_config_device() fails due to cmask being set on these bits. Its valid to update these bits and we must restore this state across migration.

Signed-off-by Jason Baron <[email protected]>

---

v2:
- Based on Michael Tsirkin's feedback:
-updated changelog 'wmask' -> 'cmask'
-Cleaned up comments
-Make cmask set more readable

 hw/pcie_aer.c |    5 +++++
 1 files changed, 5 insertions(+), 0 deletions(-)

hw/pcie_aer.c +5 -0

index 3b6981c..b04c164 100644
@@ -738,6 +738,11 @@ void pcie_aer_root_init(PCIDevice *dev)
                  PCI_ERR_ROOT_CMD_EN_MASK);
     pci_set_long(dev->w1cmask + pos + PCI_ERR_ROOT_STATUS,
                  PCI_ERR_ROOT_STATUS_REPORT_MASK);
+    /* PCI_ERR_ROOT_IRQ is RO but devices change it using a
+     * device-specific method.
+     */
+    pci_set_long(dev->cmask + pos + PCI_ERR_ROOT_STATUS,
+                 ~PCI_ERR_ROOT_IRQ);
 }
 
 void pcie_aer_root_reset(PCIDevice *dev)

Signature · 1 line

1.7.1