| Message ID | 1468244550-33910-1-git-send-email-imammedo@redhat.com |
|---|---|
| State | New |
| Headers | show |
On 11/07/2016 15:42, Igor Mammedov wrote: > this approach i I preffer as it uses less per machine migration glue > and follows typical compat pattern for devices > > Signed-off-by: Igor Mammedov <imammedo@redhat.com> > Signed-off-by: Bharata B Rao <bharata@linux.vnet.ibm.com> > --- > exec.c | 7 +++++-- > hw/i386/pc.c | 10 +++++++--- > include/hw/compat.h | 6 +++++- > include/qom/cpu.h | 3 +++ > qom/cpu.c | 13 +++++++++++++ > 5 files changed, 33 insertions(+), 6 deletions(-) Much better... Paolo
On Mon, Jul 11, 2016 at 03:42:29PM +0200, Igor Mammedov wrote: > this approach i I preffer as it uses less per machine migration glue > and follows typical compat pattern for devices > > Signed-off-by: Igor Mammedov <imammedo@redhat.com> > Signed-off-by: Bharata B Rao <bharata@linux.vnet.ibm.com> After our IRC discussion last night, I prefer this version as well. We can definitely work with this on Power as well. Reviewed-by: David Gibson <david@gibson.dropbear.id.au> One query regarding the x86 specific implementation below, though. > --- > exec.c | 7 +++++-- > hw/i386/pc.c | 10 +++++++--- > include/hw/compat.h | 6 +++++- > include/qom/cpu.h | 3 +++ > qom/cpu.c | 13 +++++++++++++ > 5 files changed, 33 insertions(+), 6 deletions(-) > > diff --git a/exec.c b/exec.c > index 0122ef7..79e1dcb 100644 > --- a/exec.c > +++ b/exec.c > @@ -670,6 +670,7 @@ void cpu_exec_init(CPUState *cpu, Error **errp) > { > CPUClass *cc = CPU_GET_CLASS(cpu); > Error *local_err = NULL; > + int migration_id; > > cpu->as = NULL; > cpu->num_ases = 0; > @@ -708,11 +709,13 @@ void cpu_exec_init(CPUState *cpu, Error **errp) > (void) cc; > cpu_list_unlock(); > #else > + migration_id = cc->use_migration_id > 0 ? > + cpu->migration_id : cpu->cpu_index; > if (qdev_get_vmsd(DEVICE(cpu)) == NULL) { > - vmstate_register(NULL, cpu->cpu_index, &vmstate_cpu_common, cpu); > + vmstate_register(NULL, migration_id, &vmstate_cpu_common, cpu); > } > if (cc->vmsd != NULL) { > - vmstate_register(NULL, cpu->cpu_index, cc->vmsd, cpu); > + vmstate_register(NULL, migration_id, cc->vmsd, cpu); > } > #endif > } > diff --git a/hw/i386/pc.c b/hw/i386/pc.c > index cd1745e..f041279 100644 > --- a/hw/i386/pc.c > +++ b/hw/i386/pc.c > @@ -1040,9 +1040,10 @@ void pc_acpi_smi_interrupt(void *opaque, int irq, int level) > } > > static X86CPU *pc_new_cpu(const char *cpu_model, int64_t apic_id, > - Error **errp) > + Error **errp, int migration_id) > { > X86CPU *cpu = NULL; > + CPUState *cs = CPU(cpu); > Error *local_err = NULL; > Any reason not to re-use the apic_id as the migration_id? It's 64-bit here which is no good, but apic_id_t is defined as uint32_t elsewhere, so is that just a bug with the function signature? > cpu = cpu_x86_create(cpu_model, &local_err); > @@ -1050,6 +1051,7 @@ static X86CPU *pc_new_cpu(const char *cpu_model, int64_t apic_id, > goto out; > } > > + cs->migration_id = migration_id; > object_property_set_int(OBJECT(cpu), apic_id, "apic-id", &local_err); > object_property_set_bool(OBJECT(cpu), true, "realized", &local_err); > > @@ -1093,7 +1095,7 @@ void pc_hot_add_cpu(const int64_t id, Error **errp) > return; > } > > - cpu = pc_new_cpu(machine->cpu_model, apic_id, &local_err); > + cpu = pc_new_cpu(machine->cpu_model, apic_id, &local_err, id); > if (local_err) { > error_propagate(errp, local_err); > return; > @@ -1137,7 +1139,7 @@ void pc_cpus_init(PCMachineState *pcms) > pcms->possible_cpus->len++; > if (i < smp_cpus) { > cpu = pc_new_cpu(machine->cpu_model, x86_cpu_apic_id_from_index(i), > - &error_fatal); > + &error_fatal, i); > pcms->possible_cpus->cpus[i].cpu = CPU(cpu); > object_unref(OBJECT(cpu)); > } > @@ -2012,7 +2014,9 @@ static void pc_machine_class_init(ObjectClass *oc, void *data) > PCMachineClass *pcmc = PC_MACHINE_CLASS(oc); > HotplugHandlerClass *hc = HOTPLUG_HANDLER_CLASS(oc); > NMIClass *nc = NMI_CLASS(oc); > + CPUClass *cc = CPU_CLASS(object_class_by_name(TYPE_CPU)); > > + cc->use_migration_id = true; > pcmc->get_hotplug_handler = mc->get_hotplug_handler; > pcmc->pci_enabled = true; > pcmc->has_acpi_build = true; > diff --git a/include/hw/compat.h b/include/hw/compat.h > index 636befe..a66e80d 100644 > --- a/include/hw/compat.h > +++ b/include/hw/compat.h > @@ -2,7 +2,11 @@ > #define HW_COMPAT_H > > #define HW_COMPAT_2_6 \ > - /* empty */ > + { \ > + .driver = TYPE_CPU, \ > + .property = "use-migration-id", \ > + .value = "off", \ > + }, > > #define HW_COMPAT_2_5 \ > {\ > diff --git a/include/qom/cpu.h b/include/qom/cpu.h > index 32f3af3..a29a2c9 100644 > --- a/include/qom/cpu.h > +++ b/include/qom/cpu.h > @@ -177,6 +177,8 @@ typedef struct CPUClass { > void *opaque); > > const struct VMStateDescription *vmsd; > + int use_migration_id; > + > int gdb_num_core_regs; > const char *gdb_core_xml_file; > gchar * (*gdb_arch_name)(CPUState *cpu); > @@ -360,6 +362,7 @@ struct CPUState { > (absolute value) offset as small as possible. This reduces code > size, especially for hosts without large memory offsets. */ > uint32_t tcg_exit_req; > + int migration_id; Not really in scope for this patch, but I do wonder if vmstate_register() should actually take a uint32_t instead of an int, since that's what it is on the wire, IIUC. > }; > > QTAILQ_HEAD(CPUTailQ, CPUState); > diff --git a/qom/cpu.c b/qom/cpu.c > index 751e992..3eb48b9 100644 > --- a/qom/cpu.c > +++ b/qom/cpu.c > @@ -342,11 +342,24 @@ static int64_t cpu_common_get_arch_id(CPUState *cpu) > return cpu->cpu_index; > } > > +static void cpu_common_set_use_migration_id(Object *obj, bool value, > + Error **err) > +{ > + CPUClass *cc = CPU_GET_CLASS(obj); > + cc->use_migration_id = value ? 1 : 0; > +} > + > static void cpu_class_init(ObjectClass *klass, void *data) > { > DeviceClass *dc = DEVICE_CLASS(klass); > CPUClass *k = CPU_CLASS(klass); > > + k->use_migration_id = -1; > + object_class_property_add_bool(klass, "use-migration-id", > + NULL, > + cpu_common_set_use_migration_id, > + &error_abort); > + > k->class_by_name = cpu_common_class_by_name; > k->parse_features = cpu_common_parse_features; > k->reset = cpu_common_reset;
On Mon, Jul 11, 2016 at 03:42:29PM +0200, Igor Mammedov wrote: > this approach i I preffer as it uses less per machine migration glue > and follows typical compat pattern for devices > > Signed-off-by: Igor Mammedov <imammedo@redhat.com> > Signed-off-by: Bharata B Rao <bharata@linux.vnet.ibm.com> > --- > exec.c | 7 +++++-- > hw/i386/pc.c | 10 +++++++--- > include/hw/compat.h | 6 +++++- > include/qom/cpu.h | 3 +++ > qom/cpu.c | 13 +++++++++++++ > 5 files changed, 33 insertions(+), 6 deletions(-) > > diff --git a/exec.c b/exec.c > index 0122ef7..79e1dcb 100644 > --- a/exec.c > +++ b/exec.c > @@ -670,6 +670,7 @@ void cpu_exec_init(CPUState *cpu, Error **errp) > { > CPUClass *cc = CPU_GET_CLASS(cpu); > Error *local_err = NULL; > + int migration_id; > > cpu->as = NULL; > cpu->num_ases = 0; > @@ -708,11 +709,13 @@ void cpu_exec_init(CPUState *cpu, Error **errp) > (void) cc; > cpu_list_unlock(); > #else > + migration_id = cc->use_migration_id > 0 ? > + cpu->migration_id : cpu->cpu_index; cc->use_migration_id is always -1 here (See explanation below) > if (qdev_get_vmsd(DEVICE(cpu)) == NULL) { > - vmstate_register(NULL, cpu->cpu_index, &vmstate_cpu_common, cpu); > + vmstate_register(NULL, migration_id, &vmstate_cpu_common, cpu); > } > if (cc->vmsd != NULL) { > - vmstate_register(NULL, cpu->cpu_index, cc->vmsd, cpu); > + vmstate_register(NULL, migration_id, cc->vmsd, cpu); > } > #endif > } > diff --git a/hw/i386/pc.c b/hw/i386/pc.c > index cd1745e..f041279 100644 > --- a/hw/i386/pc.c > +++ b/hw/i386/pc.c > @@ -1040,9 +1040,10 @@ void pc_acpi_smi_interrupt(void *opaque, int irq, int level) > } > > static X86CPU *pc_new_cpu(const char *cpu_model, int64_t apic_id, > - Error **errp) > + Error **errp, int migration_id) > { > X86CPU *cpu = NULL; > + CPUState *cs = CPU(cpu); > Error *local_err = NULL; > > cpu = cpu_x86_create(cpu_model, &local_err); Assignment to cs should move here after cpu_x86_create. > @@ -1050,6 +1051,7 @@ static X86CPU *pc_new_cpu(const char *cpu_model, int64_t apic_id, > goto out; > } > > + cs->migration_id = migration_id; > object_property_set_int(OBJECT(cpu), apic_id, "apic-id", &local_err); > object_property_set_bool(OBJECT(cpu), true, "realized", &local_err); > > @@ -1093,7 +1095,7 @@ void pc_hot_add_cpu(const int64_t id, Error **errp) > return; > } > > - cpu = pc_new_cpu(machine->cpu_model, apic_id, &local_err); > + cpu = pc_new_cpu(machine->cpu_model, apic_id, &local_err, id); > if (local_err) { > error_propagate(errp, local_err); > return; > @@ -1137,7 +1139,7 @@ void pc_cpus_init(PCMachineState *pcms) > pcms->possible_cpus->len++; > if (i < smp_cpus) { > cpu = pc_new_cpu(machine->cpu_model, x86_cpu_apic_id_from_index(i), > - &error_fatal); > + &error_fatal, i); > pcms->possible_cpus->cpus[i].cpu = CPU(cpu); > object_unref(OBJECT(cpu)); > } > @@ -2012,7 +2014,9 @@ static void pc_machine_class_init(ObjectClass *oc, void *data) > PCMachineClass *pcmc = PC_MACHINE_CLASS(oc); > HotplugHandlerClass *hc = HOTPLUG_HANDLER_CLASS(oc); > NMIClass *nc = NMI_CLASS(oc); > + CPUClass *cc = CPU_CLASS(object_class_by_name(TYPE_CPU)); > > + cc->use_migration_id = true; It is not clear as to how setting cc->use_migration_id here in machine class init would affect all CPUs. What I am observing is that the cc that we set here is different from the cc that we get via GET_CPU_CLASS(cpu) in cpu_exec_init(). Hence use_migration_id is never set for any CPUs that get created. Regards, Bharata.
On Tue, 12 Jul 2016 15:07:09 +1000 David Gibson <david@gibson.dropbear.id.au> wrote: > On Mon, Jul 11, 2016 at 03:42:29PM +0200, Igor Mammedov wrote: > > this approach i I preffer as it uses less per machine migration glue > > and follows typical compat pattern for devices > > > > Signed-off-by: Igor Mammedov <imammedo@redhat.com> > > Signed-off-by: Bharata B Rao <bharata@linux.vnet.ibm.com> > > After our IRC discussion last night, I prefer this version as well. > We can definitely work with this on Power as well. > > Reviewed-by: David Gibson <david@gibson.dropbear.id.au> Bharata, Could you take this patch but strip out PC parts which were meant as usage example only? I'll post a separate PC patch on top of it as part of or on top of cpu-hotplug for PC, as that series changes pc.c significantly and resulting PC part of patch will look a bit simpler/different. > > One query regarding the x86 specific implementation below, though. > > > --- > > exec.c | 7 +++++-- > > hw/i386/pc.c | 10 +++++++--- > > include/hw/compat.h | 6 +++++- > > include/qom/cpu.h | 3 +++ > > qom/cpu.c | 13 +++++++++++++ > > 5 files changed, 33 insertions(+), 6 deletions(-) > > > > diff --git a/exec.c b/exec.c > > index 0122ef7..79e1dcb 100644 > > --- a/exec.c > > +++ b/exec.c > > @@ -670,6 +670,7 @@ void cpu_exec_init(CPUState *cpu, Error **errp) > > { > > CPUClass *cc = CPU_GET_CLASS(cpu); > > Error *local_err = NULL; > > + int migration_id; > > > > cpu->as = NULL; > > cpu->num_ases = 0; > > @@ -708,11 +709,13 @@ void cpu_exec_init(CPUState *cpu, Error **errp) > > (void) cc; > > cpu_list_unlock(); > > #else > > + migration_id = cc->use_migration_id > 0 ? > > + cpu->migration_id : cpu->cpu_index; > > if (qdev_get_vmsd(DEVICE(cpu)) == NULL) { > > - vmstate_register(NULL, cpu->cpu_index, &vmstate_cpu_common, cpu); > > + vmstate_register(NULL, migration_id, &vmstate_cpu_common, cpu); > > } > > if (cc->vmsd != NULL) { > > - vmstate_register(NULL, cpu->cpu_index, cc->vmsd, cpu); > > + vmstate_register(NULL, migration_id, cc->vmsd, cpu); > > } > > #endif > > } > > diff --git a/hw/i386/pc.c b/hw/i386/pc.c > > index cd1745e..f041279 100644 > > --- a/hw/i386/pc.c > > +++ b/hw/i386/pc.c > > @@ -1040,9 +1040,10 @@ void pc_acpi_smi_interrupt(void *opaque, int irq, int level) > > } > > > > static X86CPU *pc_new_cpu(const char *cpu_model, int64_t apic_id, > > - Error **errp) > > + Error **errp, int migration_id) > > { > > X86CPU *cpu = NULL; > > + CPUState *cs = CPU(cpu); > > Error *local_err = NULL; > > > > Any reason not to re-use the apic_id as the migration_id? It's 64-bit > here which is no good, but apic_id_t is defined as uint32_t elsewhere, > so is that just a bug with the function signature? It could be apic_id but I chose to use possible_cpus index to make code similar as much as possible so that when I start adding cpu-hotplug to ARM it would be possible to reuse some of x86 code that could be generalized shared between both archs. > > cpu = cpu_x86_create(cpu_model, &local_err); > > @@ -1050,6 +1051,7 @@ static X86CPU *pc_new_cpu(const char *cpu_model, int64_t apic_id, > > goto out; > > } > > > > + cs->migration_id = migration_id; > > object_property_set_int(OBJECT(cpu), apic_id, "apic-id", &local_err); > > object_property_set_bool(OBJECT(cpu), true, "realized", &local_err); > > > > @@ -1093,7 +1095,7 @@ void pc_hot_add_cpu(const int64_t id, Error **errp) > > return; > > } > > > > - cpu = pc_new_cpu(machine->cpu_model, apic_id, &local_err); > > + cpu = pc_new_cpu(machine->cpu_model, apic_id, &local_err, id); > > if (local_err) { > > error_propagate(errp, local_err); > > return; > > @@ -1137,7 +1139,7 @@ void pc_cpus_init(PCMachineState *pcms) > > pcms->possible_cpus->len++; > > if (i < smp_cpus) { > > cpu = pc_new_cpu(machine->cpu_model, x86_cpu_apic_id_from_index(i), > > - &error_fatal); > > + &error_fatal, i); > > pcms->possible_cpus->cpus[i].cpu = CPU(cpu); > > object_unref(OBJECT(cpu)); > > } > > @@ -2012,7 +2014,9 @@ static void pc_machine_class_init(ObjectClass *oc, void *data) > > PCMachineClass *pcmc = PC_MACHINE_CLASS(oc); > > HotplugHandlerClass *hc = HOTPLUG_HANDLER_CLASS(oc); > > NMIClass *nc = NMI_CLASS(oc); > > + CPUClass *cc = CPU_CLASS(object_class_by_name(TYPE_CPU)); > > > > + cc->use_migration_id = true; > > pcmc->get_hotplug_handler = mc->get_hotplug_handler; > > pcmc->pci_enabled = true; > > pcmc->has_acpi_build = true; > > diff --git a/include/hw/compat.h b/include/hw/compat.h > > index 636befe..a66e80d 100644 > > --- a/include/hw/compat.h > > +++ b/include/hw/compat.h > > @@ -2,7 +2,11 @@ > > #define HW_COMPAT_H > > > > #define HW_COMPAT_2_6 \ > > - /* empty */ > > + { \ > > + .driver = TYPE_CPU, \ > > + .property = "use-migration-id", \ > > + .value = "off", \ > > + }, > > > > #define HW_COMPAT_2_5 \ > > {\ > > diff --git a/include/qom/cpu.h b/include/qom/cpu.h > > index 32f3af3..a29a2c9 100644 > > --- a/include/qom/cpu.h > > +++ b/include/qom/cpu.h > > @@ -177,6 +177,8 @@ typedef struct CPUClass { > > void *opaque); > > > > const struct VMStateDescription *vmsd; > > + int use_migration_id; > > + > > int gdb_num_core_regs; > > const char *gdb_core_xml_file; > > gchar * (*gdb_arch_name)(CPUState *cpu); > > @@ -360,6 +362,7 @@ struct CPUState { > > (absolute value) offset as small as possible. This reduces code > > size, especially for hosts without large memory offsets. */ > > uint32_t tcg_exit_req; > > + int migration_id; > > Not really in scope for this patch, but I do wonder if > vmstate_register() should actually take a uint32_t instead of an int, > since that's what it is on the wire, IIUC. I think vmstate_register() uses -1 as a way to detect if it should do instance_id auto assignment. Any way I'll leave answer to this this question to migration experts. > > > }; > > > > QTAILQ_HEAD(CPUTailQ, CPUState); > > diff --git a/qom/cpu.c b/qom/cpu.c > > index 751e992..3eb48b9 100644 > > --- a/qom/cpu.c > > +++ b/qom/cpu.c > > @@ -342,11 +342,24 @@ static int64_t cpu_common_get_arch_id(CPUState *cpu) > > return cpu->cpu_index; > > } > > > > +static void cpu_common_set_use_migration_id(Object *obj, bool value, > > + Error **err) > > +{ > > + CPUClass *cc = CPU_GET_CLASS(obj); > > + cc->use_migration_id = value ? 1 : 0; > > +} > > + > > static void cpu_class_init(ObjectClass *klass, void *data) > > { > > DeviceClass *dc = DEVICE_CLASS(klass); > > CPUClass *k = CPU_CLASS(klass); > > > > + k->use_migration_id = -1; > > + object_class_property_add_bool(klass, "use-migration-id", > > + NULL, > > + cpu_common_set_use_migration_id, > > + &error_abort); > > + > > k->class_by_name = cpu_common_class_by_name; > > k->parse_features = cpu_common_parse_features; > > k->reset = cpu_common_reset; >
On Tue, 12 Jul 2016 12:36:17 +0530 Bharata B Rao <bharata@linux.vnet.ibm.com> wrote: > On Mon, Jul 11, 2016 at 03:42:29PM +0200, Igor Mammedov wrote: > > this approach i I preffer as it uses less per machine migration glue > > and follows typical compat pattern for devices > > > > Signed-off-by: Igor Mammedov <imammedo@redhat.com> > > Signed-off-by: Bharata B Rao <bharata@linux.vnet.ibm.com> > > --- > > exec.c | 7 +++++-- > > hw/i386/pc.c | 10 +++++++--- > > include/hw/compat.h | 6 +++++- > > include/qom/cpu.h | 3 +++ > > qom/cpu.c | 13 +++++++++++++ > > 5 files changed, 33 insertions(+), 6 deletions(-) > > > > diff --git a/exec.c b/exec.c > > index 0122ef7..79e1dcb 100644 > > --- a/exec.c > > +++ b/exec.c > > @@ -670,6 +670,7 @@ void cpu_exec_init(CPUState *cpu, Error **errp) > > { > > CPUClass *cc = CPU_GET_CLASS(cpu); > > Error *local_err = NULL; > > + int migration_id; > > > > cpu->as = NULL; > > cpu->num_ases = 0; > > @@ -708,11 +709,13 @@ void cpu_exec_init(CPUState *cpu, Error **errp) > > (void) cc; > > cpu_list_unlock(); > > #else > > + migration_id = cc->use_migration_id > 0 ? > > + cpu->migration_id : cpu->cpu_index; > > cc->use_migration_id is always -1 here (See explanation below) > > > if (qdev_get_vmsd(DEVICE(cpu)) == NULL) { > > - vmstate_register(NULL, cpu->cpu_index, &vmstate_cpu_common, cpu); > > + vmstate_register(NULL, migration_id, &vmstate_cpu_common, cpu); > > } > > if (cc->vmsd != NULL) { > > - vmstate_register(NULL, cpu->cpu_index, cc->vmsd, cpu); > > + vmstate_register(NULL, migration_id, cc->vmsd, cpu); > > } > > #endif > > } > > diff --git a/hw/i386/pc.c b/hw/i386/pc.c > > index cd1745e..f041279 100644 > > --- a/hw/i386/pc.c > > +++ b/hw/i386/pc.c > > @@ -1040,9 +1040,10 @@ void pc_acpi_smi_interrupt(void *opaque, int irq, int level) > > } > > > > static X86CPU *pc_new_cpu(const char *cpu_model, int64_t apic_id, > > - Error **errp) > > + Error **errp, int migration_id) > > { > > X86CPU *cpu = NULL; > > + CPUState *cs = CPU(cpu); > > Error *local_err = NULL; > > > > cpu = cpu_x86_create(cpu_model, &local_err); > > Assignment to cs should move here after cpu_x86_create. thanks, I'll fix it, though there won't be this hunk in at all in the end PC version in top of cpu-hotplug series. > > > @@ -1050,6 +1051,7 @@ static X86CPU *pc_new_cpu(const char *cpu_model, int64_t apic_id, > > goto out; > > } > > > > + cs->migration_id = migration_id; > > object_property_set_int(OBJECT(cpu), apic_id, "apic-id", &local_err); > > object_property_set_bool(OBJECT(cpu), true, "realized", &local_err); > > > > @@ -1093,7 +1095,7 @@ void pc_hot_add_cpu(const int64_t id, Error **errp) > > return; > > } > > > > - cpu = pc_new_cpu(machine->cpu_model, apic_id, &local_err); > > + cpu = pc_new_cpu(machine->cpu_model, apic_id, &local_err, id); > > if (local_err) { > > error_propagate(errp, local_err); > > return; > > @@ -1137,7 +1139,7 @@ void pc_cpus_init(PCMachineState *pcms) > > pcms->possible_cpus->len++; > > if (i < smp_cpus) { > > cpu = pc_new_cpu(machine->cpu_model, x86_cpu_apic_id_from_index(i), > > - &error_fatal); > > + &error_fatal, i); > > pcms->possible_cpus->cpus[i].cpu = CPU(cpu); > > object_unref(OBJECT(cpu)); > > } > > @@ -2012,7 +2014,9 @@ static void pc_machine_class_init(ObjectClass *oc, void *data) > > PCMachineClass *pcmc = PC_MACHINE_CLASS(oc); > > HotplugHandlerClass *hc = HOTPLUG_HANDLER_CLASS(oc); > > NMIClass *nc = NMI_CLASS(oc); > > + CPUClass *cc = CPU_CLASS(object_class_by_name(TYPE_CPU)); > > > > + cc->use_migration_id = true; > > It is not clear as to how setting cc->use_migration_id here in machine class > init would affect all CPUs. What I am observing is that the cc that we > set here is different from the cc that we get via GET_CPU_CLASS(cpu) in > cpu_exec_init(). Hence use_migration_id is never set for any CPUs that > get created. It look like inherited types are initialized before this is called, I'll try find solution and post fixed patch shortly. > > Regards, > Bharata. >
On Tue, Jul 12, 2016 at 10:11:58AM +0200, Igor Mammedov wrote: > On Tue, 12 Jul 2016 15:07:09 +1000 > David Gibson <david@gibson.dropbear.id.au> wrote: > > > On Mon, Jul 11, 2016 at 03:42:29PM +0200, Igor Mammedov wrote: > > > this approach i I preffer as it uses less per machine migration glue > > > and follows typical compat pattern for devices > > > > > > Signed-off-by: Igor Mammedov <imammedo@redhat.com> > > > Signed-off-by: Bharata B Rao <bharata@linux.vnet.ibm.com> > > > > After our IRC discussion last night, I prefer this version as well. > > We can definitely work with this on Power as well. > > > > Reviewed-by: David Gibson <david@gibson.dropbear.id.au> > > Bharata, > > Could you take this patch but strip out PC parts which were meant > as usage example only? > > I'll post a separate PC patch on top of it as part of or on top of > cpu-hotplug for PC, as that series changes pc.c significantly and > resulting PC part of patch will look a bit simpler/different. > > > > > One query regarding the x86 specific implementation below, though. > > > > > --- > > > exec.c | 7 +++++-- > > > hw/i386/pc.c | 10 +++++++--- > > > include/hw/compat.h | 6 +++++- > > > include/qom/cpu.h | 3 +++ > > > qom/cpu.c | 13 +++++++++++++ > > > 5 files changed, 33 insertions(+), 6 deletions(-) > > > > > > diff --git a/exec.c b/exec.c > > > index 0122ef7..79e1dcb 100644 > > > --- a/exec.c > > > +++ b/exec.c > > > @@ -670,6 +670,7 @@ void cpu_exec_init(CPUState *cpu, Error **errp) > > > { > > > CPUClass *cc = CPU_GET_CLASS(cpu); > > > Error *local_err = NULL; > > > + int migration_id; > > > > > > cpu->as = NULL; > > > cpu->num_ases = 0; > > > @@ -708,11 +709,13 @@ void cpu_exec_init(CPUState *cpu, Error **errp) > > > (void) cc; > > > cpu_list_unlock(); > > > #else > > > + migration_id = cc->use_migration_id > 0 ? > > > + cpu->migration_id : cpu->cpu_index; > > > if (qdev_get_vmsd(DEVICE(cpu)) == NULL) { > > > - vmstate_register(NULL, cpu->cpu_index, &vmstate_cpu_common, cpu); > > > + vmstate_register(NULL, migration_id, &vmstate_cpu_common, cpu); > > > } > > > if (cc->vmsd != NULL) { > > > - vmstate_register(NULL, cpu->cpu_index, cc->vmsd, cpu); > > > + vmstate_register(NULL, migration_id, cc->vmsd, cpu); > > > } > > > #endif > > > } > > > diff --git a/hw/i386/pc.c b/hw/i386/pc.c > > > index cd1745e..f041279 100644 > > > --- a/hw/i386/pc.c > > > +++ b/hw/i386/pc.c > > > @@ -1040,9 +1040,10 @@ void pc_acpi_smi_interrupt(void *opaque, int irq, int level) > > > } > > > > > > static X86CPU *pc_new_cpu(const char *cpu_model, int64_t apic_id, > > > - Error **errp) > > > + Error **errp, int migration_id) > > > { > > > X86CPU *cpu = NULL; > > > + CPUState *cs = CPU(cpu); > > > Error *local_err = NULL; > > > > > > > Any reason not to re-use the apic_id as the migration_id? It's 64-bit > > here which is no good, but apic_id_t is defined as uint32_t elsewhere, > > so is that just a bug with the function signature? > It could be apic_id but I chose to use possible_cpus index to make code > similar as much as possible so that when I start adding cpu-hotplug to ARM > it would be possible to reuse some of x86 code that could be generalized > shared between both archs. Ok. > > > cpu = cpu_x86_create(cpu_model, &local_err); > > > @@ -1050,6 +1051,7 @@ static X86CPU *pc_new_cpu(const char *cpu_model, int64_t apic_id, > > > goto out; > > > } > > > > > > + cs->migration_id = migration_id; > > > object_property_set_int(OBJECT(cpu), apic_id, "apic-id", &local_err); > > > object_property_set_bool(OBJECT(cpu), true, "realized", &local_err); > > > > > > @@ -1093,7 +1095,7 @@ void pc_hot_add_cpu(const int64_t id, Error **errp) > > > return; > > > } > > > > > > - cpu = pc_new_cpu(machine->cpu_model, apic_id, &local_err); > > > + cpu = pc_new_cpu(machine->cpu_model, apic_id, &local_err, id); > > > if (local_err) { > > > error_propagate(errp, local_err); > > > return; > > > @@ -1137,7 +1139,7 @@ void pc_cpus_init(PCMachineState *pcms) > > > pcms->possible_cpus->len++; > > > if (i < smp_cpus) { > > > cpu = pc_new_cpu(machine->cpu_model, x86_cpu_apic_id_from_index(i), > > > - &error_fatal); > > > + &error_fatal, i); > > > pcms->possible_cpus->cpus[i].cpu = CPU(cpu); > > > object_unref(OBJECT(cpu)); > > > } > > > @@ -2012,7 +2014,9 @@ static void pc_machine_class_init(ObjectClass *oc, void *data) > > > PCMachineClass *pcmc = PC_MACHINE_CLASS(oc); > > > HotplugHandlerClass *hc = HOTPLUG_HANDLER_CLASS(oc); > > > NMIClass *nc = NMI_CLASS(oc); > > > + CPUClass *cc = CPU_CLASS(object_class_by_name(TYPE_CPU)); > > > > > > + cc->use_migration_id = true; > > > pcmc->get_hotplug_handler = mc->get_hotplug_handler; > > > pcmc->pci_enabled = true; > > > pcmc->has_acpi_build = true; > > > diff --git a/include/hw/compat.h b/include/hw/compat.h > > > index 636befe..a66e80d 100644 > > > --- a/include/hw/compat.h > > > +++ b/include/hw/compat.h > > > @@ -2,7 +2,11 @@ > > > #define HW_COMPAT_H > > > > > > #define HW_COMPAT_2_6 \ > > > - /* empty */ > > > + { \ > > > + .driver = TYPE_CPU, \ > > > + .property = "use-migration-id", \ > > > + .value = "off", \ > > > + }, > > > > > > #define HW_COMPAT_2_5 \ > > > {\ > > > diff --git a/include/qom/cpu.h b/include/qom/cpu.h > > > index 32f3af3..a29a2c9 100644 > > > --- a/include/qom/cpu.h > > > +++ b/include/qom/cpu.h > > > @@ -177,6 +177,8 @@ typedef struct CPUClass { > > > void *opaque); > > > > > > const struct VMStateDescription *vmsd; > > > + int use_migration_id; > > > + > > > int gdb_num_core_regs; > > > const char *gdb_core_xml_file; > > > gchar * (*gdb_arch_name)(CPUState *cpu); > > > @@ -360,6 +362,7 @@ struct CPUState { > > > (absolute value) offset as small as possible. This reduces code > > > size, especially for hosts without large memory offsets. */ > > > uint32_t tcg_exit_req; > > > + int migration_id; > > > > Not really in scope for this patch, but I do wonder if > > vmstate_register() should actually take a uint32_t instead of an int, > > since that's what it is on the wire, IIUC. > I think vmstate_register() uses -1 as a way to detect if it should > do instance_id auto assignment. > Any way I'll leave answer to this this question to migration > > experts. Ah, good point.
diff --git a/exec.c b/exec.c index 0122ef7..79e1dcb 100644 --- a/exec.c +++ b/exec.c @@ -670,6 +670,7 @@ void cpu_exec_init(CPUState *cpu, Error **errp) { CPUClass *cc = CPU_GET_CLASS(cpu); Error *local_err = NULL; + int migration_id; cpu->as = NULL; cpu->num_ases = 0; @@ -708,11 +709,13 @@ void cpu_exec_init(CPUState *cpu, Error **errp) (void) cc; cpu_list_unlock(); #else + migration_id = cc->use_migration_id > 0 ? + cpu->migration_id : cpu->cpu_index; if (qdev_get_vmsd(DEVICE(cpu)) == NULL) { - vmstate_register(NULL, cpu->cpu_index, &vmstate_cpu_common, cpu); + vmstate_register(NULL, migration_id, &vmstate_cpu_common, cpu); } if (cc->vmsd != NULL) { - vmstate_register(NULL, cpu->cpu_index, cc->vmsd, cpu); + vmstate_register(NULL, migration_id, cc->vmsd, cpu); } #endif } diff --git a/hw/i386/pc.c b/hw/i386/pc.c index cd1745e..f041279 100644 --- a/hw/i386/pc.c +++ b/hw/i386/pc.c @@ -1040,9 +1040,10 @@ void pc_acpi_smi_interrupt(void *opaque, int irq, int level) } static X86CPU *pc_new_cpu(const char *cpu_model, int64_t apic_id, - Error **errp) + Error **errp, int migration_id) { X86CPU *cpu = NULL; + CPUState *cs = CPU(cpu); Error *local_err = NULL; cpu = cpu_x86_create(cpu_model, &local_err); @@ -1050,6 +1051,7 @@ static X86CPU *pc_new_cpu(const char *cpu_model, int64_t apic_id, goto out; } + cs->migration_id = migration_id; object_property_set_int(OBJECT(cpu), apic_id, "apic-id", &local_err); object_property_set_bool(OBJECT(cpu), true, "realized", &local_err); @@ -1093,7 +1095,7 @@ void pc_hot_add_cpu(const int64_t id, Error **errp) return; } - cpu = pc_new_cpu(machine->cpu_model, apic_id, &local_err); + cpu = pc_new_cpu(machine->cpu_model, apic_id, &local_err, id); if (local_err) { error_propagate(errp, local_err); return; @@ -1137,7 +1139,7 @@ void pc_cpus_init(PCMachineState *pcms) pcms->possible_cpus->len++; if (i < smp_cpus) { cpu = pc_new_cpu(machine->cpu_model, x86_cpu_apic_id_from_index(i), - &error_fatal); + &error_fatal, i); pcms->possible_cpus->cpus[i].cpu = CPU(cpu); object_unref(OBJECT(cpu)); } @@ -2012,7 +2014,9 @@ static void pc_machine_class_init(ObjectClass *oc, void *data) PCMachineClass *pcmc = PC_MACHINE_CLASS(oc); HotplugHandlerClass *hc = HOTPLUG_HANDLER_CLASS(oc); NMIClass *nc = NMI_CLASS(oc); + CPUClass *cc = CPU_CLASS(object_class_by_name(TYPE_CPU)); + cc->use_migration_id = true; pcmc->get_hotplug_handler = mc->get_hotplug_handler; pcmc->pci_enabled = true; pcmc->has_acpi_build = true; diff --git a/include/hw/compat.h b/include/hw/compat.h index 636befe..a66e80d 100644 --- a/include/hw/compat.h +++ b/include/hw/compat.h @@ -2,7 +2,11 @@ #define HW_COMPAT_H #define HW_COMPAT_2_6 \ - /* empty */ + { \ + .driver = TYPE_CPU, \ + .property = "use-migration-id", \ + .value = "off", \ + }, #define HW_COMPAT_2_5 \ {\ diff --git a/include/qom/cpu.h b/include/qom/cpu.h index 32f3af3..a29a2c9 100644 --- a/include/qom/cpu.h +++ b/include/qom/cpu.h @@ -177,6 +177,8 @@ typedef struct CPUClass { void *opaque); const struct VMStateDescription *vmsd; + int use_migration_id; + int gdb_num_core_regs; const char *gdb_core_xml_file; gchar * (*gdb_arch_name)(CPUState *cpu); @@ -360,6 +362,7 @@ struct CPUState { (absolute value) offset as small as possible. This reduces code size, especially for hosts without large memory offsets. */ uint32_t tcg_exit_req; + int migration_id; }; QTAILQ_HEAD(CPUTailQ, CPUState); diff --git a/qom/cpu.c b/qom/cpu.c index 751e992..3eb48b9 100644 --- a/qom/cpu.c +++ b/qom/cpu.c @@ -342,11 +342,24 @@ static int64_t cpu_common_get_arch_id(CPUState *cpu) return cpu->cpu_index; } +static void cpu_common_set_use_migration_id(Object *obj, bool value, + Error **err) +{ + CPUClass *cc = CPU_GET_CLASS(obj); + cc->use_migration_id = value ? 1 : 0; +} + static void cpu_class_init(ObjectClass *klass, void *data) { DeviceClass *dc = DEVICE_CLASS(klass); CPUClass *k = CPU_CLASS(klass); + k->use_migration_id = -1; + object_class_property_add_bool(klass, "use-migration-id", + NULL, + cpu_common_set_use_migration_id, + &error_abort); + k->class_by_name = cpu_common_class_by_name; k->parse_features = cpu_common_parse_features; k->reset = cpu_common_reset;