Re: [Qemu-devel] [PATCH 12/12] target-i386: implement CPU hot-add

[Igor Mammedov <imammedo@redhat.com>]

On Wed, 27 Mar 2013 12:19:01 +0100
Paolo Bonzini <pbonzini@redhat.com> wrote:

> Il 21/03/2013 15:28, Igor Mammedov ha scritto:
> > ... via do_cpu_hot_add() hook called by cpu_set QMP command,
> > for x86 target.
> > 
> > * add extra check that APIC ID is in allowed range
> > * return error if CPU with requested APIC ID exists before creating
> >   a new instance. (CPU removal is broken now, will be fixed with CPU unplug)
> > * call CPU add notifier as the last step of x86_cpu_realizefn() to
> >   update rtc_cmos and trigger CPU hot-plug ACPI GPE to notify guest.
> >   Doing it from x86_cpu_realizefn() will allow to do the same when
> >   it would be possible to add CPU using device_add.
> > 
> > Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> > ---
> >  hw/i386/pc.c      |   22 ++++++++++++++++++++++
> >  target-i386/cpu.c |    1 +
> >  2 files changed, 23 insertions(+), 0 deletions(-)
> > 
> > diff --git a/hw/i386/pc.c b/hw/i386/pc.c
> > index 7481f73..e3ba9ee 100644
> > --- a/hw/i386/pc.c
> > +++ b/hw/i386/pc.c
> > @@ -867,6 +867,19 @@ static void pc_new_cpu(const char *cpu_model, int64_t apic_id, Error **errp)
> >  {
> >      X86CPU *cpu;
> >  
> > +    if (apic_id >= pc_apic_id_limit(max_cpus)) {
> > +        error_setg(errp, "Unable to add CPU with ID: 0x%" PRIx64
> > +                   ", max allowed ID: 0x%x", apic_id,
> > +                   pc_apic_id_limit(max_cpus) - 1);
> > +        return;
> > +    }
> 
> This seems the wrong place to do this check.  It should be done in
> do_cpu_hot_add, simply comparing against max_cpus.  Here, instead, you
> should _assert_ that the APIC ID is in range.
I'll move it there,  but I'd leave pc_apic_id_limit(max_cpus) since id it
compares with is APIC ID.

> 
> > +    if (x86_cpu_is_cpu_exist(qdev_get_machine(), &apic_id)) {
> 
> Similarly, can this be done in qmp_cpu_set?  And should it really be an
I've tried to abstract qmp part from target/implementation details.
We could introduce global func like, and then use it if from QMP subsystem:
  bool is_cpu_exist(int64_t id) {
     ... iterate over CPUs ...
     if (cpu->get_firmware_id() == id)
         return true;
     ...
  }

Andreas, Is it acceptable if I add it to qom/cpu.h, qom/cpu.c ?

> error?  Onlining an already-online CPU is fine.
CPU is not just onlined though, it's created and it couldn't be right to
create the same CPU. Hence a error here, so user would know that it tries to
add the same device.

> 
> Again, here you could assert that the CPU is not a duplicate, instead.
sure

> 
> > +        error_setg(errp, "Unable to add CPU with ID: 0x%" PRIx64
> > +                   ", it's already exists", apic_id);
> > +        return;
> > +    }
> >      cpu = cpu_x86_create(cpu_model, errp);
> >      if (!cpu) {
> >          return;
> > @@ -882,6 +895,8 @@ static void pc_new_cpu(const char *cpu_model, int64_t apic_id, Error **errp)
> >      }
> >  }
> >  
> > +static const char *saved_cpu_model;
> > +
> >  void pc_cpus_init(const char *cpu_model)
> 
> Instead of using this global, see the approach in the previous patch.
> 
> >  {
> >      int i;
> > @@ -895,6 +910,8 @@ void pc_cpus_init(const char *cpu_model)
> >          cpu_model = "qemu32";
> >  #endif
> >      }
> > +    saved_cpu_model = cpu_model;
> > +
> 
> >      for (i = 0; i < smp_cpus; i++) {
> >          pc_new_cpu(cpu_model, x86_cpu_apic_id_from_index(i), &error);
> > @@ -906,6 +923,11 @@ void pc_cpus_init(const char *cpu_model)
> >      }
> >  }
> >  
> > +void do_cpu_hot_add(const int64_t id, Error **errp)
> > +{
> > +    pc_new_cpu(saved_cpu_model, id, errp);
> > +}
> > +
> 
> Missing x86_cpu_apic_id_from_index(id)?
There was(is?) opposition to using cpu_index to identify x86 CPU.
So, it is expected from management to provide APIC ID instead of cpu_index.
It could be useful to make hotplug to a specific NUMA node/cpu to work in
future.
Though interface of possible APIC IDs discovery is not part of this series.

> 
> >  void pc_acpi_init(const char *default_dsdt)
> >  {
> >      char *filename = NULL, *arg = NULL;
> > diff --git a/target-i386/cpu.c b/target-i386/cpu.c
> > index ae46f81..d127141 100644
> > --- a/target-i386/cpu.c
> > +++ b/target-i386/cpu.c
> > @@ -2271,6 +2271,7 @@ out:
> >      if (dev->hotplugged) {
> >          cpu_synchronize_post_init(env);
> >          resume_vcpu(CPU(cpu));
> > +        qemu_system_cpu_hotplug_request(env->cpuid_apic_id);
> 
> As mentioned earlier, this notifier should be invoked at the CPU level,
> not X86CPU.
done

> Paolo
> 
> >      }
> >  }
> >  
> > 
> 
> 


-- 
Regards,
  Igor