[PATCH v2 0/4] apic: Fix migration breakage of >255 vcpus

[PATCH v2 0/4] apic: Fix migration breakage of >255 vcpus

[Peter Xu]

v2:
- use uint32_t rather than int64_t [Juan]
- one more patch (patch 4) to check dup SaveStateEntry [Dave]
- one more patch to define a macro (patch 1) to simplify patch 2

Please review, thanks.

Peter Xu (4):
  migration: Define VMSTATE_INSTANCE_ID_ANY
  migration: Change SaveStateEntry.instance_id into uint32_t
  apic: Use 32bit APIC ID for migration instance ID
  migration: Check in savevm_state_handler_insert for dups

 hw/arm/stellaris.c           |  2 +-
 hw/core/qdev.c               |  3 ++-
 hw/display/ads7846.c         |  2 +-
 hw/i2c/core.c                |  2 +-
 hw/input/stellaris_input.c   |  3 ++-
 hw/intc/apic_common.c        |  7 +++++--
 hw/misc/max111x.c            |  2 +-
 hw/net/eepro100.c            |  2 +-
 hw/pci/pci.c                 |  2 +-
 hw/ppc/spapr.c               |  2 +-
 hw/timer/arm_timer.c         |  2 +-
 hw/tpm/tpm_emulator.c        |  3 ++-
 include/migration/register.h |  2 +-
 include/migration/vmstate.h  |  6 ++++--
 migration/savevm.c           | 40 +++++++++++++++++++++++++-----------
 stubs/vmstate.c              |  2 +-
 16 files changed, 53 insertions(+), 29 deletions(-)

-- 
2.21.0

[PATCH v2 1/4] migration: Define VMSTATE_INSTANCE_ID_ANY

[Peter Xu]

Define the new macro VMSTATE_INSTANCE_ID_ANY for callers who wants to
auto-generate the vmstate instance ID.  Previously it was hard coded
as -1 instead of this macro.  It helps to change this default value in
the follow up patches.  No functional change.

Signed-off-by: Peter Xu <peterx@redhat.com>
---
 hw/arm/stellaris.c          | 2 +-
 hw/core/qdev.c              | 3 ++-
 hw/display/ads7846.c        | 2 +-
 hw/i2c/core.c               | 2 +-
 hw/input/stellaris_input.c  | 3 ++-
 hw/intc/apic_common.c       | 2 +-
 hw/misc/max111x.c           | 2 +-
 hw/net/eepro100.c           | 2 +-
 hw/pci/pci.c                | 2 +-
 hw/ppc/spapr.c              | 2 +-
 hw/timer/arm_timer.c        | 2 +-
 hw/tpm/tpm_emulator.c       | 3 ++-
 include/migration/vmstate.h | 2 ++
 migration/savevm.c          | 8 ++++----
 14 files changed, 21 insertions(+), 16 deletions(-)

diff --git a/hw/arm/stellaris.c b/hw/arm/stellaris.c
index b198066b54..bb025e0bd0 100644
--- a/hw/arm/stellaris.c
+++ b/hw/arm/stellaris.c
@@ -708,7 +708,7 @@ static int stellaris_sys_init(uint32_t base, qemu_irq irq,
     memory_region_init_io(&s->iomem, NULL, &ssys_ops, s, "ssys", 0x00001000);
     memory_region_add_subregion(get_system_memory(), base, &s->iomem);
     ssys_reset(s);
-    vmstate_register(NULL, -1, &vmstate_stellaris_sys, s);
+    vmstate_register(NULL, VMSTATE_INSTANCE_ID_ANY, &vmstate_stellaris_sys, s);
     return 0;
 }
 
diff --git a/hw/core/qdev.c b/hw/core/qdev.c
index cbad6c1d55..86031f961d 100644
--- a/hw/core/qdev.c
+++ b/hw/core/qdev.c
@@ -866,7 +866,8 @@ static void device_set_realized(Object *obj, bool value, Error **errp)
         dev->canonical_path = object_get_canonical_path(OBJECT(dev));
 
         if (qdev_get_vmsd(dev)) {
-            if (vmstate_register_with_alias_id(dev, -1, qdev_get_vmsd(dev), dev,
+            if (vmstate_register_with_alias_id(dev, VMSTATE_INSTANCE_ID_ANY,
+                                               qdev_get_vmsd(dev), dev,
                                                dev->instance_id_alias,
                                                dev->alias_required_for_version,
                                                &local_err) < 0) {
diff --git a/hw/display/ads7846.c b/hw/display/ads7846.c
index c12272ae72..9228b40b1a 100644
--- a/hw/display/ads7846.c
+++ b/hw/display/ads7846.c
@@ -154,7 +154,7 @@ static void ads7846_realize(SSISlave *d, Error **errp)
 
     ads7846_int_update(s);
 
-    vmstate_register(NULL, -1, &vmstate_ads7846, s);
+    vmstate_register(NULL, VMSTATE_INSTANCE_ID_ANY, &vmstate_ads7846, s);
 }
 
 static void ads7846_class_init(ObjectClass *klass, void *data)
diff --git a/hw/i2c/core.c b/hw/i2c/core.c
index 92cd489069..d770035ba0 100644
--- a/hw/i2c/core.c
+++ b/hw/i2c/core.c
@@ -61,7 +61,7 @@ I2CBus *i2c_init_bus(DeviceState *parent, const char *name)
 
     bus = I2C_BUS(qbus_create(TYPE_I2C_BUS, parent, name));
     QLIST_INIT(&bus->current_devs);
-    vmstate_register(NULL, -1, &vmstate_i2c_bus, bus);
+    vmstate_register(NULL, VMSTATE_INSTANCE_ID_ANY, &vmstate_i2c_bus, bus);
     return bus;
 }
 
diff --git a/hw/input/stellaris_input.c b/hw/input/stellaris_input.c
index 59892b07fc..e6ee5e11f1 100644
--- a/hw/input/stellaris_input.c
+++ b/hw/input/stellaris_input.c
@@ -88,5 +88,6 @@ void stellaris_gamepad_init(int n, qemu_irq *irq, const int *keycode)
     }
     s->num_buttons = n;
     qemu_add_kbd_event_handler(stellaris_gamepad_put_key, s);
-    vmstate_register(NULL, -1, &vmstate_stellaris_gamepad, s);
+    vmstate_register(NULL, VMSTATE_INSTANCE_ID_ANY,
+                     &vmstate_stellaris_gamepad, s);
 }
diff --git a/hw/intc/apic_common.c b/hw/intc/apic_common.c
index aafd8e0e33..22da53ce8a 100644
--- a/hw/intc/apic_common.c
+++ b/hw/intc/apic_common.c
@@ -331,7 +331,7 @@ static void apic_common_realize(DeviceState *dev, Error **errp)
     }
 
     if (s->legacy_instance_id) {
-        instance_id = -1;
+        instance_id = VMSTATE_INSTANCE_ID_ANY;
     }
     vmstate_register_with_alias_id(NULL, instance_id, &vmstate_apic_common,
                                    s, -1, 0, NULL);
diff --git a/hw/misc/max111x.c b/hw/misc/max111x.c
index a713149f16..81ee73e0da 100644
--- a/hw/misc/max111x.c
+++ b/hw/misc/max111x.c
@@ -146,7 +146,7 @@ static int max111x_init(SSISlave *d, int inputs)
     s->input[7] = 0x80;
     s->com = 0;
 
-    vmstate_register(dev, -1, &vmstate_max111x, s);
+    vmstate_register(dev, VMSTATE_INSTANCE_ID_ANY, &vmstate_max111x, s);
     return 0;
 }
 
diff --git a/hw/net/eepro100.c b/hw/net/eepro100.c
index cc2dd8b1c9..39920c6dc5 100644
--- a/hw/net/eepro100.c
+++ b/hw/net/eepro100.c
@@ -1874,7 +1874,7 @@ static void e100_nic_realize(PCIDevice *pci_dev, Error **errp)
 
     s->vmstate = g_memdup(&vmstate_eepro100, sizeof(vmstate_eepro100));
     s->vmstate->name = qemu_get_queue(s->nic)->model;
-    vmstate_register(&pci_dev->qdev, -1, s->vmstate, s);
+    vmstate_register(&pci_dev->qdev, VMSTATE_INSTANCE_ID_ANY, s->vmstate, s);
 }
 
 static void eepro100_instance_init(Object *obj)
diff --git a/hw/pci/pci.c b/hw/pci/pci.c
index aa05c2b9b2..708f66aac3 100644
--- a/hw/pci/pci.c
+++ b/hw/pci/pci.c
@@ -122,7 +122,7 @@ static void pci_bus_realize(BusState *qbus, Error **errp)
     bus->machine_done.notify = pcibus_machine_done;
     qemu_add_machine_init_done_notifier(&bus->machine_done);
 
-    vmstate_register(NULL, -1, &vmstate_pcibus, bus);
+    vmstate_register(NULL, VMSTATE_INSTANCE_ID_ANY, &vmstate_pcibus, bus);
 }
 
 static void pcie_bus_realize(BusState *qbus, Error **errp)
diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
index 514a17ae74..a316399ae9 100644
--- a/hw/ppc/spapr.c
+++ b/hw/ppc/spapr.c
@@ -3029,7 +3029,7 @@ static void spapr_machine_init(MachineState *machine)
      * interface, this is a legacy from the sPAPREnvironment structure
      * which predated MachineState but had a similar function */
     vmstate_register(NULL, 0, &vmstate_spapr, spapr);
-    register_savevm_live("spapr/htab", -1, 1,
+    register_savevm_live("spapr/htab", VMSTATE_INSTANCE_ID_ANY, 1,
                          &savevm_htab_handlers, spapr);
 
     qbus_set_hotplug_handler(sysbus_get_default(), OBJECT(machine),
diff --git a/hw/timer/arm_timer.c b/hw/timer/arm_timer.c
index c2e6211188..1e9dea451a 100644
--- a/hw/timer/arm_timer.c
+++ b/hw/timer/arm_timer.c
@@ -174,7 +174,7 @@ static arm_timer_state *arm_timer_init(uint32_t freq)
 
     bh = qemu_bh_new(arm_timer_tick, s);
     s->timer = ptimer_init(bh, PTIMER_POLICY_DEFAULT);
-    vmstate_register(NULL, -1, &vmstate_arm_timer, s);
+    vmstate_register(NULL, VMSTATE_INSTANCE_ID_ANY, &vmstate_arm_timer, s);
     return s;
 }
 
diff --git a/hw/tpm/tpm_emulator.c b/hw/tpm/tpm_emulator.c
index 22f9113432..da7b490489 100644
--- a/hw/tpm/tpm_emulator.c
+++ b/hw/tpm/tpm_emulator.c
@@ -914,7 +914,8 @@ static void tpm_emulator_inst_init(Object *obj)
     tpm_emu->cur_locty_number = ~0;
     qemu_mutex_init(&tpm_emu->mutex);
 
-    vmstate_register(NULL, -1, &vmstate_tpm_emulator, obj);
+    vmstate_register(NULL, VMSTATE_INSTANCE_ID_ANY,
+                     &vmstate_tpm_emulator, obj);
 }
 
 /*
diff --git a/include/migration/vmstate.h b/include/migration/vmstate.h
index 1fbfd099dd..c551470299 100644
--- a/include/migration/vmstate.h
+++ b/include/migration/vmstate.h
@@ -1113,6 +1113,8 @@ int vmstate_save_state_v(QEMUFile *f, const VMStateDescription *vmsd,
 
 bool vmstate_save_needed(const VMStateDescription *vmsd, void *opaque);
 
+#define  VMSTATE_INSTANCE_ID_ANY  -1
+
 /* Returns: 0 on success, -1 on failure */
 int vmstate_register_with_alias_id(DeviceState *dev, int instance_id,
                                    const VMStateDescription *vmsd,
diff --git a/migration/savevm.c b/migration/savevm.c
index bb9462a54d..0074572a52 100644
--- a/migration/savevm.c
+++ b/migration/savevm.c
@@ -750,7 +750,7 @@ int register_savevm_live(const char *idstr,
 
     pstrcat(se->idstr, sizeof(se->idstr), idstr);
 
-    if (instance_id == -1) {
+    if (instance_id == VMSTATE_INSTANCE_ID_ANY) {
         se->instance_id = calculate_new_instance_id(se->idstr);
     } else {
         se->instance_id = instance_id;
@@ -817,14 +817,14 @@ int vmstate_register_with_alias_id(DeviceState *dev, int instance_id,
 
             se->compat = g_new0(CompatEntry, 1);
             pstrcpy(se->compat->idstr, sizeof(se->compat->idstr), vmsd->name);
-            se->compat->instance_id = instance_id == -1 ?
+            se->compat->instance_id = instance_id == VMSTATE_INSTANCE_ID_ANY ?
                          calculate_compat_instance_id(vmsd->name) : instance_id;
-            instance_id = -1;
+            instance_id = VMSTATE_INSTANCE_ID_ANY;
         }
     }
     pstrcat(se->idstr, sizeof(se->idstr), vmsd->name);
 
-    if (instance_id == -1) {
+    if (instance_id == VMSTATE_INSTANCE_ID_ANY) {
         se->instance_id = calculate_new_instance_id(se->idstr);
     } else {
         se->instance_id = instance_id;
-- 
2.21.0

Re: [PATCH v2 1/4] migration: Define VMSTATE_INSTANCE_ID_ANY

[Juan Quintela]

Peter Xu <peterx@redhat.com> wrote:
> Define the new macro VMSTATE_INSTANCE_ID_ANY for callers who wants to
> auto-generate the vmstate instance ID.  Previously it was hard coded
> as -1 instead of this macro.  It helps to change this default value in
> the follow up patches.  No functional change.
>
> Signed-off-by: Peter Xu <peterx@redhat.com>

Reviewed-by: Juan Quintela <quintela@redhat.com>

[PATCH v2 2/4] migration: Change SaveStateEntry.instance_id into uint32_t

[Peter Xu]

It was always used as 32bit, so define it as used to be clear.
Instead of using -1 as the auto-gen magic value, we switch to
UINT32_MAX.  We also make sure that we don't auto-gen this value to
avoid overflowed instance IDs without being noticed.

Suggested-by: Juan Quintela <quintela@redhat.com>
Signed-off-by: Peter Xu <peterx@redhat.com>
---
 hw/intc/apic_common.c        |  2 +-
 include/migration/register.h |  2 +-
 include/migration/vmstate.h  |  6 +++---
 migration/savevm.c           | 18 ++++++++++--------
 stubs/vmstate.c              |  2 +-
 5 files changed, 16 insertions(+), 14 deletions(-)

diff --git a/hw/intc/apic_common.c b/hw/intc/apic_common.c
index 22da53ce8a..fabfa7320b 100644
--- a/hw/intc/apic_common.c
+++ b/hw/intc/apic_common.c
@@ -315,7 +315,7 @@ static void apic_common_realize(DeviceState *dev, Error **errp)
     APICCommonState *s = APIC_COMMON(dev);
     APICCommonClass *info;
     static DeviceState *vapic;
-    int instance_id = s->id;
+    uint32_t instance_id = s->id;
 
     info = APIC_COMMON_GET_CLASS(s);
     info->realize(dev, errp);
diff --git a/include/migration/register.h b/include/migration/register.h
index a13359a08d..f3ba10b6ef 100644
--- a/include/migration/register.h
+++ b/include/migration/register.h
@@ -69,7 +69,7 @@ typedef struct SaveVMHandlers {
 } SaveVMHandlers;
 
 int register_savevm_live(const char *idstr,
-                         int instance_id,
+                         uint32_t instance_id,
                          int version_id,
                          const SaveVMHandlers *ops,
                          void *opaque);
diff --git a/include/migration/vmstate.h b/include/migration/vmstate.h
index c551470299..67bc63e30e 100644
--- a/include/migration/vmstate.h
+++ b/include/migration/vmstate.h
@@ -1113,17 +1113,17 @@ int vmstate_save_state_v(QEMUFile *f, const VMStateDescription *vmsd,
 
 bool vmstate_save_needed(const VMStateDescription *vmsd, void *opaque);
 
-#define  VMSTATE_INSTANCE_ID_ANY  -1
+#define  VMSTATE_INSTANCE_ID_ANY  UINT32_MAX
 
 /* Returns: 0 on success, -1 on failure */
-int vmstate_register_with_alias_id(DeviceState *dev, int instance_id,
+int vmstate_register_with_alias_id(DeviceState *dev, uint32_t instance_id,
                                    const VMStateDescription *vmsd,
                                    void *base, int alias_id,
                                    int required_for_version,
                                    Error **errp);
 
 /* Returns: 0 on success, -1 on failure */
-static inline int vmstate_register(DeviceState *dev, int instance_id,
+static inline int vmstate_register(DeviceState *dev, uint32_t instance_id,
                                    const VMStateDescription *vmsd,
                                    void *opaque)
 {
diff --git a/migration/savevm.c b/migration/savevm.c
index 0074572a52..1e44f06d7a 100644
--- a/migration/savevm.c
+++ b/migration/savevm.c
@@ -233,7 +233,7 @@ typedef struct CompatEntry {
 typedef struct SaveStateEntry {
     QTAILQ_ENTRY(SaveStateEntry) entry;
     char idstr[256];
-    int instance_id;
+    uint32_t instance_id;
     int alias_id;
     int version_id;
     /* version id read from the stream */
@@ -665,10 +665,10 @@ void dump_vmstate_json_to_file(FILE *out_file)
     fclose(out_file);
 }
 
-static int calculate_new_instance_id(const char *idstr)
+static uint32_t calculate_new_instance_id(const char *idstr)
 {
     SaveStateEntry *se;
-    int instance_id = 0;
+    uint32_t instance_id = 0;
 
     QTAILQ_FOREACH(se, &savevm_state.handlers, entry) {
         if (strcmp(idstr, se->idstr) == 0
@@ -676,6 +676,8 @@ static int calculate_new_instance_id(const char *idstr)
             instance_id = se->instance_id + 1;
         }
     }
+    /* Make sure we never loop over without being noticed */
+    assert(instance_id != VMSTATE_INSTANCE_ID_ANY);
     return instance_id;
 }
 
@@ -730,7 +732,7 @@ static void savevm_state_handler_insert(SaveStateEntry *nse)
    Meanwhile pass -1 as instance_id if you do not already have a clearly
    distinguishing id for all instances of your device class. */
 int register_savevm_live(const char *idstr,
-                         int instance_id,
+                         uint32_t instance_id,
                          int version_id,
                          const SaveVMHandlers *ops,
                          void *opaque)
@@ -784,7 +786,7 @@ void unregister_savevm(DeviceState *dev, const char *idstr, void *opaque)
     }
 }
 
-int vmstate_register_with_alias_id(DeviceState *dev, int instance_id,
+int vmstate_register_with_alias_id(DeviceState *dev, uint32_t instance_id,
                                    const VMStateDescription *vmsd,
                                    void *opaque, int alias_id,
                                    int required_for_version,
@@ -1566,7 +1568,7 @@ int qemu_save_device_state(QEMUFile *f)
     return qemu_file_get_error(f);
 }
 
-static SaveStateEntry *find_se(const char *idstr, int instance_id)
+static SaveStateEntry *find_se(const char *idstr, uint32_t instance_id)
 {
     SaveStateEntry *se;
 
@@ -2235,7 +2237,7 @@ qemu_loadvm_section_start_full(QEMUFile *f, MigrationIncomingState *mis)
     /* Find savevm section */
     se = find_se(idstr, instance_id);
     if (se == NULL) {
-        error_report("Unknown savevm section or instance '%s' %d. "
+        error_report("Unknown savevm section or instance '%s' %"PRIu32". "
                      "Make sure that your current VM setup matches your "
                      "saved VM setup, including any hotplugged devices",
                      idstr, instance_id);
@@ -2259,7 +2261,7 @@ qemu_loadvm_section_start_full(QEMUFile *f, MigrationIncomingState *mis)
 
     ret = vmstate_load(f, se);
     if (ret < 0) {
-        error_report("error while loading state for instance 0x%x of"
+        error_report("error while loading state for instance 0x%"PRIx32" of"
                      " device '%s'", instance_id, idstr);
         return ret;
     }
diff --git a/stubs/vmstate.c b/stubs/vmstate.c
index e1e89b87f0..4ed5cc6e9e 100644
--- a/stubs/vmstate.c
+++ b/stubs/vmstate.c
@@ -4,7 +4,7 @@
 const VMStateDescription vmstate_dummy = {};
 
 int vmstate_register_with_alias_id(DeviceState *dev,
-                                   int instance_id,
+                                   uint32_t instance_id,
                                    const VMStateDescription *vmsd,
                                    void *base, int alias_id,
                                    int required_for_version,
-- 
2.21.0

Re: [PATCH v2 2/4] migration: Change SaveStateEntry.instance_id into uint32_t

[Juan Quintela]

Peter Xu <peterx@redhat.com> wrote:
> It was always used as 32bit, so define it as used to be clear.
> Instead of using -1 as the auto-gen magic value, we switch to
> UINT32_MAX.  We also make sure that we don't auto-gen this value to
> avoid overflowed instance IDs without being noticed.
>
> Suggested-by: Juan Quintela <quintela@redhat.com>
> Signed-off-by: Peter Xu <peterx@redhat.com>

Reviewed-by: Juan Quintela <quintela@redhat.com>

[PATCH v2 3/4] apic: Use 32bit APIC ID for migration instance ID

[Peter Xu]

Migration is silently broken now with x2apic config like this:

     -smp 200,maxcpus=288,sockets=2,cores=72,threads=2 \
     -device intel-iommu,intremap=on,eim=on

After migration, the guest kernel could hang at anything, due to
x2apic bit not migrated correctly in IA32_APIC_BASE on some vcpus, so
any operations related to x2apic could be broken then (e.g., RDMSR on
x2apic MSRs could fail because KVM would think that the vcpu hasn't
enabled x2apic at all).

The issue is that the x2apic bit was never applied correctly for vcpus
whose ID > 255 when migrate completes, and that's because when we
migrate APIC we use the APICCommonState.id as instance ID of the
migration stream, while that's too short for x2apic.

Let's use the newly introduced initial_apic_id for that.

Signed-off-by: Peter Xu <peterx@redhat.com>
---
 hw/intc/apic_common.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/hw/intc/apic_common.c b/hw/intc/apic_common.c
index fabfa7320b..f0d88a1b14 100644
--- a/hw/intc/apic_common.c
+++ b/hw/intc/apic_common.c
@@ -315,7 +315,10 @@ static void apic_common_realize(DeviceState *dev, Error **errp)
     APICCommonState *s = APIC_COMMON(dev);
     APICCommonClass *info;
     static DeviceState *vapic;
-    uint32_t instance_id = s->id;
+    uint32_t instance_id = s->initial_apic_id;
+
+    /* Normally initial APIC ID should be no more than hundreds */
+    assert(instance_id != VMSTATE_INSTANCE_ID_ANY);
 
     info = APIC_COMMON_GET_CLASS(s);
     info->realize(dev, errp);
-- 
2.21.0

Re: [PATCH v2 3/4] apic: Use 32bit APIC ID for migration instance ID

[Eduardo Habkost]

On Wed, Oct 16, 2019 at 10:29:32AM +0800, Peter Xu wrote:
> Migration is silently broken now with x2apic config like this:
> 
>      -smp 200,maxcpus=288,sockets=2,cores=72,threads=2 \
>      -device intel-iommu,intremap=on,eim=on
> 
> After migration, the guest kernel could hang at anything, due to
> x2apic bit not migrated correctly in IA32_APIC_BASE on some vcpus, so
> any operations related to x2apic could be broken then (e.g., RDMSR on
> x2apic MSRs could fail because KVM would think that the vcpu hasn't
> enabled x2apic at all).
> 
> The issue is that the x2apic bit was never applied correctly for vcpus
> whose ID > 255 when migrate completes, and that's because when we
> migrate APIC we use the APICCommonState.id as instance ID of the
> migration stream, while that's too short for x2apic.
> 
> Let's use the newly introduced initial_apic_id for that.
> 
> Signed-off-by: Peter Xu <peterx@redhat.com>

Reviewed-by: Eduardo Habkost <ehabkost@redhat.com>

-- 
Eduardo

Re: [PATCH v2 3/4] apic: Use 32bit APIC ID for migration instance ID

[Juan Quintela]

Peter Xu <peterx@redhat.com> wrote:
> Migration is silently broken now with x2apic config like this:
>
>      -smp 200,maxcpus=288,sockets=2,cores=72,threads=2 \
>      -device intel-iommu,intremap=on,eim=on
>
> After migration, the guest kernel could hang at anything, due to
> x2apic bit not migrated correctly in IA32_APIC_BASE on some vcpus, so
> any operations related to x2apic could be broken then (e.g., RDMSR on
> x2apic MSRs could fail because KVM would think that the vcpu hasn't
> enabled x2apic at all).
>
> The issue is that the x2apic bit was never applied correctly for vcpus
> whose ID > 255 when migrate completes, and that's because when we
> migrate APIC we use the APICCommonState.id as instance ID of the
> migration stream, while that's too short for x2apic.
>
> Let's use the newly introduced initial_apic_id for that.
>
> Signed-off-by: Peter Xu <peterx@redhat.com>

Reviewed-by: Juan Quintela <quintela@redhat.com>

[PATCH v2 4/4] migration: Check in savevm_state_handler_insert for dups

[Peter Xu]

Before finally register one SaveStateEntry, we detect for duplicated
entries.  This could be helpful to notify us asap instead of get
silent migration failures which could be hard to diagnose.

For example, this patch will generate a message like this (if without
previous fixes on x2apic) as long as we wants to boot a VM instance
with "-smp 200,maxcpus=288,sockets=2,cores=72,threads=2" and QEMU will
bail out even before VM starts:

savevm_state_handler_insert: Detected duplicate SaveStateEntry: id=apic, instance_id=0x0

Suggested-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
Signed-off-by: Peter Xu <peterx@redhat.com>
---
 migration/savevm.c | 14 ++++++++++++++
 1 file changed, 14 insertions(+)

diff --git a/migration/savevm.c b/migration/savevm.c
index 1e44f06d7a..83e91ddafa 100644
--- a/migration/savevm.c
+++ b/migration/savevm.c
@@ -264,6 +264,8 @@ static SaveState savevm_state = {
     .global_section_id = 0,
 };
 
+static SaveStateEntry *find_se(const char *idstr, uint32_t instance_id);
+
 static bool should_validate_capability(int capability)
 {
     assert(capability >= 0 && capability < MIGRATION_CAPABILITY__MAX);
@@ -714,6 +716,18 @@ static void savevm_state_handler_insert(SaveStateEntry *nse)
 
     assert(priority <= MIG_PRI_MAX);
 
+    /*
+     * This should never happen otherwise migration will probably fail
+     * silently somewhere because we can be wrongly applying one
+     * object properties upon another one.  Bail out ASAP.
+     */
+    if (find_se(nse->idstr, nse->instance_id)) {
+        error_report("%s: Detected duplicate SaveStateEntry: "
+                     "id=%s, instance_id=0x%"PRIx32, __func__,
+                     nse->idstr, nse->instance_id);
+        exit(EXIT_FAILURE);
+    }
+
     QTAILQ_FOREACH(se, &savevm_state.handlers, entry) {
         if (save_state_priority(se) < priority) {
             break;
-- 
2.21.0

Re: [PATCH v2 4/4] migration: Check in savevm_state_handler_insert for dups

[Dr. David Alan Gilbert]

* Peter Xu (peterx@redhat.com) wrote:
> Before finally register one SaveStateEntry, we detect for duplicated
> entries.  This could be helpful to notify us asap instead of get
> silent migration failures which could be hard to diagnose.
> 
> For example, this patch will generate a message like this (if without
> previous fixes on x2apic) as long as we wants to boot a VM instance
> with "-smp 200,maxcpus=288,sockets=2,cores=72,threads=2" and QEMU will
> bail out even before VM starts:
> 
> savevm_state_handler_insert: Detected duplicate SaveStateEntry: id=apic, instance_id=0x0
> 
> Suggested-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
> Signed-off-by: Peter Xu <peterx@redhat.com>

Right, lets see what this triggers :-)

Reviewed-by: Dr. David Alan Gilbert <dgilbert@redhat.com>

> ---
>  migration/savevm.c | 14 ++++++++++++++
>  1 file changed, 14 insertions(+)
> 
> diff --git a/migration/savevm.c b/migration/savevm.c
> index 1e44f06d7a..83e91ddafa 100644
> --- a/migration/savevm.c
> +++ b/migration/savevm.c
> @@ -264,6 +264,8 @@ static SaveState savevm_state = {
>      .global_section_id = 0,
>  };
>  
> +static SaveStateEntry *find_se(const char *idstr, uint32_t instance_id);
> +
>  static bool should_validate_capability(int capability)
>  {
>      assert(capability >= 0 && capability < MIGRATION_CAPABILITY__MAX);
> @@ -714,6 +716,18 @@ static void savevm_state_handler_insert(SaveStateEntry *nse)
>  
>      assert(priority <= MIG_PRI_MAX);
>  
> +    /*
> +     * This should never happen otherwise migration will probably fail
> +     * silently somewhere because we can be wrongly applying one
> +     * object properties upon another one.  Bail out ASAP.
> +     */
> +    if (find_se(nse->idstr, nse->instance_id)) {
> +        error_report("%s: Detected duplicate SaveStateEntry: "
> +                     "id=%s, instance_id=0x%"PRIx32, __func__,
> +                     nse->idstr, nse->instance_id);
> +        exit(EXIT_FAILURE);
> +    }
> +
>      QTAILQ_FOREACH(se, &savevm_state.handlers, entry) {
>          if (save_state_priority(se) < priority) {
>              break;
> -- 
> 2.21.0
> 
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

Re: [PATCH v2 4/4] migration: Check in savevm_state_handler_insert for dups

[Juan Quintela]

Peter Xu <peterx@redhat.com> wrote:
> Before finally register one SaveStateEntry, we detect for duplicated
> entries.  This could be helpful to notify us asap instead of get
> silent migration failures which could be hard to diagnose.
>
> For example, this patch will generate a message like this (if without
> previous fixes on x2apic) as long as we wants to boot a VM instance
> with "-smp 200,maxcpus=288,sockets=2,cores=72,threads=2" and QEMU will
> bail out even before VM starts:
>
> savevm_state_handler_insert: Detected duplicate SaveStateEntry: id=apic, instance_id=0x0
>
> Suggested-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
> Signed-off-by: Peter Xu <peterx@redhat.com>

Reviewed-by: Juan Quintela <quintela@redhat.com>

Re: [PATCH v2 4/4] migration: Check in savevm_state_handler_insert for dups

[Juan Quintela]

Peter Xu <peterx@redhat.com> wrote:
> Before finally register one SaveStateEntry, we detect for duplicated
> entries.  This could be helpful to notify us asap instead of get
> silent migration failures which could be hard to diagnose.
>
> For example, this patch will generate a message like this (if without
> previous fixes on x2apic) as long as we wants to boot a VM instance
> with "-smp 200,maxcpus=288,sockets=2,cores=72,threads=2" and QEMU will
> bail out even before VM starts:
>
> savevm_state_handler_insert: Detected duplicate SaveStateEntry: id=apic, instance_id=0x0
>
> Suggested-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
> Signed-off-by: Peter Xu <peterx@redhat.com>

Hi peter

I have to drop this one.  There is something on current tree (I think
that it is the VMSTATE_IF) that make isa-ide choke for migration on
"make check".  Will take a look to it later.

Later, Juan.

Re: [PATCH v2 0/4] apic: Fix migration breakage of >255 vcpus

[Eduardo Habkost]

On Wed, Oct 16, 2019 at 10:29:29AM +0800, Peter Xu wrote:
> v2:
> - use uint32_t rather than int64_t [Juan]
> - one more patch (patch 4) to check dup SaveStateEntry [Dave]
> - one more patch to define a macro (patch 1) to simplify patch 2
> 
> Please review, thanks.

I wonder how hard it is to write a simple test case to reproduce
the original bug.  We can extend tests/migration-test.c or
tests/acceptance/migration.py.  If using -device with explicit
apic-id, we probably don't even need to create >255 VCPUs.

> 
> Peter Xu (4):
>   migration: Define VMSTATE_INSTANCE_ID_ANY
>   migration: Change SaveStateEntry.instance_id into uint32_t
>   apic: Use 32bit APIC ID for migration instance ID
>   migration: Check in savevm_state_handler_insert for dups
> 
>  hw/arm/stellaris.c           |  2 +-
>  hw/core/qdev.c               |  3 ++-
>  hw/display/ads7846.c         |  2 +-
>  hw/i2c/core.c                |  2 +-
>  hw/input/stellaris_input.c   |  3 ++-
>  hw/intc/apic_common.c        |  7 +++++--
>  hw/misc/max111x.c            |  2 +-
>  hw/net/eepro100.c            |  2 +-
>  hw/pci/pci.c                 |  2 +-
>  hw/ppc/spapr.c               |  2 +-
>  hw/timer/arm_timer.c         |  2 +-
>  hw/tpm/tpm_emulator.c        |  3 ++-
>  include/migration/register.h |  2 +-
>  include/migration/vmstate.h  |  6 ++++--
>  migration/savevm.c           | 40 +++++++++++++++++++++++++-----------
>  stubs/vmstate.c              |  2 +-
>  16 files changed, 53 insertions(+), 29 deletions(-)
> 
> -- 
> 2.21.0
> 

-- 
Eduardo

Re: [PATCH v2 0/4] apic: Fix migration breakage of >255 vcpus

[Peter Xu]

On Wed, Oct 16, 2019 at 11:40:01AM -0300, Eduardo Habkost wrote:
> On Wed, Oct 16, 2019 at 10:29:29AM +0800, Peter Xu wrote:
> > v2:
> > - use uint32_t rather than int64_t [Juan]
> > - one more patch (patch 4) to check dup SaveStateEntry [Dave]
> > - one more patch to define a macro (patch 1) to simplify patch 2
> > 
> > Please review, thanks.
> 
> I wonder how hard it is to write a simple test case to reproduce
> the original bug.  We can extend tests/migration-test.c or
> tests/acceptance/migration.py.  If using -device with explicit
> apic-id, we probably don't even need to create >255 VCPUs.

I can give it a shot next week. :)

-- 
Peter Xu

Re: [PATCH v2 0/4] apic: Fix migration breakage of >255 vcpus

[Peter Xu]

On Sat, Oct 19, 2019 at 11:41:53AM +0800, Peter Xu wrote:
> On Wed, Oct 16, 2019 at 11:40:01AM -0300, Eduardo Habkost wrote:
> > On Wed, Oct 16, 2019 at 10:29:29AM +0800, Peter Xu wrote:
> > > v2:
> > > - use uint32_t rather than int64_t [Juan]
> > > - one more patch (patch 4) to check dup SaveStateEntry [Dave]
> > > - one more patch to define a macro (patch 1) to simplify patch 2
> > > 
> > > Please review, thanks.
> > 
> > I wonder how hard it is to write a simple test case to reproduce
> > the original bug.  We can extend tests/migration-test.c or
> > tests/acceptance/migration.py.  If using -device with explicit
> > apic-id, we probably don't even need to create >255 VCPUs.
> 
> I can give it a shot next week. :)

When trying this, I probably noticed a block layer issue: q35 seems to
have problem on booting from a very small block device (like 512B,
which is the image size that currently used for migration-test.c).
For example, this cmdline can boot successfully into the test image:

$qemu -M pc -m 200m -accel kvm -nographic \
      -drive file=$image,id=drive0,index=0,format=raw \
      -device ide-hd,drive=drive0

While this cannot:

$qemu -M q35 -m 200m -accel kvm -nographic \
      -drive file=$image,id=drive0,index=0,format=raw \
      -device ide-hd,drive=drive0

With error (BIOS debug messages on):

Booting from Hard Disk..invalid basic_access:143:
   a=00000201  b=00000000  c=00000001  d=00000080 ds=0000 es=07c0 ss=d980
  si=00000000 di=00000000 bp=00000000 sp=0000fd8e cs=f000 ip=cb81  f=0202
invalid basic_access:144:
   a=00000201  b=00000000  c=00000001  d=00000080 ds=0000 es=07c0 ss=d980
  si=00000000 di=00000000 bp=00000000 sp=0000fd8e cs=f000 ip=cb81  f=0202
.
Boot failed: could not read the boot disenter handle_18:
  NULL
k

This corresponds to this SeaBIOS check error:

static void noinline
basic_access(struct bregs *regs, struct drive_s *drive_fl, u16 command)
{
    ...
    // sanity check on cyl heads, sec
    if (cylinder >= nlc || head >= nlh || sector > nls) {
        warn_invalid(regs);
        disk_ret(regs, DISK_RET_EPARAM);
        return;
    }
    ...
}

And... below cmdline will work even for q35 (as suggested by Fam when
we talked offline):

$qemu -M q35 -m 200m -accel kvm -nographic \
      -drive file=$image,id=drive0,index=0,format=raw \
      -device ide-hd,drive=drive0,secs=1,cyls=1,heads=1

I think for migration test we can workaround like above, but I'm also
curious whether this is a real bug somewhere because I don't see a
reason for q35 to refuse to boot on a one-sector image.

Thanks,

-- 
Peter Xu

Re: [PATCH v2 0/4] apic: Fix migration breakage of >255 vcpus

[Kevin Wolf]

Am 23.10.2019 um 09:57 hat Peter Xu geschrieben:
> On Sat, Oct 19, 2019 at 11:41:53AM +0800, Peter Xu wrote:
> > On Wed, Oct 16, 2019 at 11:40:01AM -0300, Eduardo Habkost wrote:
> > > On Wed, Oct 16, 2019 at 10:29:29AM +0800, Peter Xu wrote:
> > > > v2:
> > > > - use uint32_t rather than int64_t [Juan]
> > > > - one more patch (patch 4) to check dup SaveStateEntry [Dave]
> > > > - one more patch to define a macro (patch 1) to simplify patch 2
> > > > 
> > > > Please review, thanks.
> > > 
> > > I wonder how hard it is to write a simple test case to reproduce
> > > the original bug.  We can extend tests/migration-test.c or
> > > tests/acceptance/migration.py.  If using -device with explicit
> > > apic-id, we probably don't even need to create >255 VCPUs.
> > 
> > I can give it a shot next week. :)
> 
> When trying this, I probably noticed a block layer issue: q35 seems to
> have problem on booting from a very small block device (like 512B,
> which is the image size that currently used for migration-test.c).
> For example, this cmdline can boot successfully into the test image:
> 
> $qemu -M pc -m 200m -accel kvm -nographic \
>       -drive file=$image,id=drive0,index=0,format=raw \
>       -device ide-hd,drive=drive0
> 
> While this cannot:
> 
> $qemu -M q35 -m 200m -accel kvm -nographic \
>       -drive file=$image,id=drive0,index=0,format=raw \
>       -device ide-hd,drive=drive0

The important difference here is legacy IDE (which works) vs. AHCI
(which doesn't work). If you add a -device ahci to the -M pc case, it
starts failing, too.

Not sure why AHCI fails, but I'll just CC John who is the lucky
maintainer of this device. :-)

Kevin

> With error (BIOS debug messages on):
> 
> Booting from Hard Disk..invalid basic_access:143:
>    a=00000201  b=00000000  c=00000001  d=00000080 ds=0000 es=07c0 ss=d980
>   si=00000000 di=00000000 bp=00000000 sp=0000fd8e cs=f000 ip=cb81  f=0202
> invalid basic_access:144:
>    a=00000201  b=00000000  c=00000001  d=00000080 ds=0000 es=07c0 ss=d980
>   si=00000000 di=00000000 bp=00000000 sp=0000fd8e cs=f000 ip=cb81  f=0202
> .
> Boot failed: could not read the boot disenter handle_18:
>   NULL
> k
> 
> This corresponds to this SeaBIOS check error:
> 
> static void noinline
> basic_access(struct bregs *regs, struct drive_s *drive_fl, u16 command)
> {
>     ...
>     // sanity check on cyl heads, sec
>     if (cylinder >= nlc || head >= nlh || sector > nls) {
>         warn_invalid(regs);
>         disk_ret(regs, DISK_RET_EPARAM);
>         return;
>     }
>     ...
> }
> 
> And... below cmdline will work even for q35 (as suggested by Fam when
> we talked offline):
> 
> $qemu -M q35 -m 200m -accel kvm -nographic \
>       -drive file=$image,id=drive0,index=0,format=raw \
>       -device ide-hd,drive=drive0,secs=1,cyls=1,heads=1
> 
> I think for migration test we can workaround like above, but I'm also
> curious whether this is a real bug somewhere because I don't see a
> reason for q35 to refuse to boot on a one-sector image.
> 
> Thanks,
> 
> -- 
> Peter Xu
> 

Re: [PATCH v2 0/4] apic: Fix migration breakage of >255 vcpus

[John Snow]

On 10/23/19 4:17 AM, Kevin Wolf wrote:
> The important difference here is legacy IDE (which works) vs. AHCI
> (which doesn't work). If you add a -device ahci to the -M pc case, it
> starts failing, too.
> 
> Not sure why AHCI fails, but I'll just CC John who is the lucky
> maintainer of this device. :-)

Hm... It looks like SeaBIOS is identifying the drive correctly and
perfectly well, but we're failing at boot_disk(u8 bootdrv, int
checksig), about here:

call16_int(0x13, &br);

if (br.flags & F_CF) {
    printf("Boot failed: could not read the boot disk\n\n");
    return;
}

Looking at AHCI tracing (From the QEMU side), it looks like we set up
the drive correctly, and then never touch the port ever again -- I don't
see an attempted read on QEMU's end.

I'll need to look through SeaBIOS source for hints, I'm not sure right
yet. If anyone is more familiar with the SeaBIOS boot code, maybe they
can give a pointer faster than I'll figure it out myself.

--js

Re: [PATCH v2 0/4] apic: Fix migration breakage of >255 vcpus

[Peter Xu]

On Thu, Oct 24, 2019 at 01:49:11PM -0400, John Snow wrote:
> 
> 
> On 10/23/19 4:17 AM, Kevin Wolf wrote:
> > The important difference here is legacy IDE (which works) vs. AHCI
> > (which doesn't work). If you add a -device ahci to the -M pc case, it
> > starts failing, too.
> > 
> > Not sure why AHCI fails, but I'll just CC John who is the lucky
> > maintainer of this device. :-)
> 
> Hm... It looks like SeaBIOS is identifying the drive correctly and
> perfectly well, but we're failing at boot_disk(u8 bootdrv, int
> checksig), about here:
> 
> call16_int(0x13, &br);
> 
> if (br.flags & F_CF) {
>     printf("Boot failed: could not read the boot disk\n\n");
>     return;
> }
> 
> Looking at AHCI tracing (From the QEMU side), it looks like we set up
> the drive correctly, and then never touch the port ever again -- I don't
> see an attempted read on QEMU's end.
> 
> I'll need to look through SeaBIOS source for hints, I'm not sure right
> yet. If anyone is more familiar with the SeaBIOS boot code, maybe they
> can give a pointer faster than I'll figure it out myself.

Hi, John,

I don't know seabios well, but I did have a pointer in my previous
email on where it faulted.  It seems to me that the issue is that
SeaBIOS may have got incorrect secs/cyls/heads information (and
explicitly setting secs=1,cyls=1,heads=1 on the block device fixes the
issue).

Thanks,

-- 
Peter Xu

Re: [PATCH v2 0/4] apic: Fix migration breakage of >255 vcpus

[Peter Xu]

On Sat, Oct 19, 2019 at 11:41:53AM +0800, Peter Xu wrote:
> On Wed, Oct 16, 2019 at 11:40:01AM -0300, Eduardo Habkost wrote:
> > On Wed, Oct 16, 2019 at 10:29:29AM +0800, Peter Xu wrote:
> > > v2:
> > > - use uint32_t rather than int64_t [Juan]
> > > - one more patch (patch 4) to check dup SaveStateEntry [Dave]
> > > - one more patch to define a macro (patch 1) to simplify patch 2
> > > 
> > > Please review, thanks.
> > 
> > I wonder how hard it is to write a simple test case to reproduce
> > the original bug.  We can extend tests/migration-test.c or
> > tests/acceptance/migration.py.  If using -device with explicit
> > apic-id, we probably don't even need to create >255 VCPUs.
> 
> I can give it a shot next week. :)

When I was playing with it, I noticed that it's not that easy at least
for the migration-test.  We need to do these:

- add one specific CPU with apic-id>255, this is easy by using
  "-device qemu64-x86_64-cpu,..."

- enable x2apic in the guest code, read apic-id on the special vcpu to
  make sure it's correct even after migration, but before that...

- I failed to find a way to use apic-id>255 as the BSP of system but I
  can only create APs with apic-id>255, so we need to add initial MP
  support for the migration guest code, then run that apic-id check
  code on the new AP

- I also probably found that q35 bug on bootstraping the 512B disk, so
  we probably need to workaround that too until fixed

Unless someone has better idea on this, I'll simply stop here because
I'm afraid it does not worth the effort so far... (or until we have
some other requirement to enrich the migration qtest framework)

-- 
Peter Xu