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

[Paolo Bonzini <pbonzini@redhat.com>]

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.

> +    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
error?  Onlining an already-online CPU is fine.

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

> +        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)?

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

Paolo

>      }
>  }
>  
>