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

[David Gibson]

On Tue, Jul 12, 2016 at 10:11:58AM +0200, Igor Mammedov wrote:
> On Tue, 12 Jul 2016 15:07:09 +1000
> David Gibson <address@hidden> 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 <address@hidden>
> > > Signed-off-by: Bharata B Rao <address@hidden>  
> > 
> > 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 <address@hidden>
> 
> 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.

-- 
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

signature.asc
Description: PGP signature