Re: [Qemu-devel] [PATCH] VARIANT 1: reuse device compat logic to pick preffered CPU's migration instance_id

[David Gibson <david@gibson.dropbear.id.au>]

[-- Attachment #1: Type: text/plain, Size: 6708 bytes --]

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;

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]