Re: [Qemu-devel] [PATCH v2] cpu: Correct cpu-hotplug failure

[Igor Mammedov <imammedo@redhat.com>]

On Thu, 25 Jul 2013 17:49:11 +0800
Chen Fan <chen.fan.fnst@cn.fujitsu.com> wrote:

> v1-v2: Change cpu_apic_realize to post_vcpu_init.
> 
> When useing x86_64-softmmu --enable-kvm boot qemu, cpu-add command fails to add a vcpu,
       ^^^ using
> there show (KVM: setting VAPIC address failed).
> 
> The reason is that we use an uninitialized cpu->kvm-fd to ioctl.
> so we move realizing apic to the back of qemu_init_vcpu.
Could you describe here why patch fixes problem and how it's different
from non hotplug case. So anyone touching this code in future
could know what dependencies to check.

> 
> Signed-off-by: Chen Fan <chen.fan.fnst@cn.fujitsu.com>
> ---
>  include/qom/cpu.h |  2 ++
>  qom/cpu.c         | 13 +++++++++++++
>  target-i386/cpu.c | 10 ++++------
>  3 files changed, 19 insertions(+), 6 deletions(-)
> 
> diff --git a/include/qom/cpu.h b/include/qom/cpu.h
> index daf1835..4b16385 100644
> --- a/include/qom/cpu.h
> +++ b/include/qom/cpu.h
> @@ -80,6 +80,7 @@ struct TranslationBlock;
>   * @synchronize_from_tb: Callback for synchronizing state from a TCG
>   * #TranslationBlock.
>   * @get_phys_page_debug: Callback for obtaining a physical address.
> + * @post_vcpu_init: Callback for doing some extra initialization.
>   * @vmsd: State description for migration.
>   *
>   * Represents a CPU family or model.
> @@ -108,6 +109,7 @@ typedef struct CPUClass {
>      void (*set_pc)(CPUState *cpu, vaddr value);
>      void (*synchronize_from_tb)(CPUState *cpu, struct TranslationBlock *tb);
>      hwaddr (*get_phys_page_debug)(CPUState *cpu, vaddr addr);
> +    void (*post_vcpu_init)(CPUState *cpu, Error **errp);
>  
>      const struct VMStateDescription *vmsd;
>      int (*write_elf64_note)(WriteCoreDumpFunction f, CPUState *cpu,
> diff --git a/qom/cpu.c b/qom/cpu.c
> index 5c45ab5..28f63b7 100644
> --- a/qom/cpu.c
> +++ b/qom/cpu.c
> @@ -213,12 +213,25 @@ static ObjectClass *cpu_common_class_by_name(const char *cpu_model)
>      return NULL;
>  }
>  
> +static void post_vcpu_init(CPUState *cpu, Error **errp)
> +{
> +    CPUClass *cc = CPU_GET_CLASS(cpu);
> +    if (cc->post_vcpu_init != NULL) {
> +        (*cc->post_vcpu_init)(cpu, errp);
> +    }
> +}
> +
>  static void cpu_common_realizefn(DeviceState *dev, Error **errp)
>  {
>      CPUState *cpu = CPU(dev);
>  
>      qemu_init_vcpu(cpu);
>  
> +    post_vcpu_init(cpu, errp);
> +    if (error_is_set(errp)) {
> +        return;
> +    }
> +
>      if (dev->hotplugged) {
>          cpu_synchronize_post_init(cpu);
>          notifier_list_notify(&cpu_added_notifiers, dev);
> diff --git a/target-i386/cpu.c b/target-i386/cpu.c
> index cd350cb..d51ab8b 100644
> --- a/target-i386/cpu.c
> +++ b/target-i386/cpu.c
> @@ -2311,8 +2311,9 @@ static void x86_cpu_apic_create(X86CPU *cpu, Error **errp)
>      apic->cpu = cpu;
>  }
>  
> -static void x86_cpu_apic_realize(X86CPU *cpu, Error **errp)
> +static void x86_cpu_apic_realize(CPUState *s, Error **errp)
>  {
> +    X86CPU *cpu = X86_CPU(s);
>      CPUX86State *env = &cpu->env;
>  
>      if (env->apic_state == NULL) {
> @@ -2326,7 +2327,7 @@ static void x86_cpu_apic_realize(X86CPU *cpu, Error **errp)
>      }
>  }
>  #else
> -static void x86_cpu_apic_realize(X86CPU *cpu, Error **errp)
> +static void x86_cpu_apic_realize(CPUState *s, Error **errp)
>  {
>  }
>  #endif
> @@ -2388,10 +2389,6 @@ static void x86_cpu_realizefn(DeviceState *dev, Error **errp)
>  
>      mce_init(cpu);
>  
> -    x86_cpu_apic_realize(cpu, &local_err);
> -    if (local_err != NULL) {
> -        goto out;
> -    }
>      cpu_reset(CPU(cpu));
>  
>      xcc->parent_realize(dev, &local_err);
> @@ -2540,6 +2537,7 @@ static void x86_cpu_common_class_init(ObjectClass *oc, void *data)
>      cc->synchronize_from_tb = x86_cpu_synchronize_from_tb;
>      cc->get_arch_id = x86_cpu_get_arch_id;
>      cc->get_paging_enabled = x86_cpu_get_paging_enabled;
> +    cc->post_vcpu_init = x86_cpu_apic_realize;
>  #ifndef CONFIG_USER_ONLY
>      cc->get_memory_mapping = x86_cpu_get_memory_mapping;
>      cc->get_phys_page_debug = x86_cpu_get_phys_page_debug;