[Qemu-devel] [PATCH 1/1] cpu: Correct cpu-hotplug failure

[Qemu-devel] [PATCH 1/1] cpu: Correct cpu-hotplug failure

[Chen Fan]

When useing x86_64-softmmu --enable-kvm boot qemu, cpu-add command fails to add a vcpu,
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.

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..487a808 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.
+ * @apic_realize: Callback for realizing apic.
  * @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 (*apic_realize)(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..88c6028 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 cpu_apic_realize(CPUState *cpu, Error **errp)
+{
+    CPUClass *cc = CPU_GET_CLASS(cpu);
+    if (cc->apic_realize != NULL) {
+        (*cc->apic_realize)(cpu, errp);
+    }
+}
+
 static void cpu_common_realizefn(DeviceState *dev, Error **errp)
 {
     CPUState *cpu = CPU(dev);
 
     qemu_init_vcpu(cpu);
 
+    cpu_apic_realize(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..916d69e 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->apic_realize = 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;
-- 
1.8.1.4

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

[chenfan]

On Thu, 2013-07-25 at 14:27 +0800, Chen Fan wrote:
> When useing x86_64-softmmu --enable-kvm boot qemu, cpu-add command fails to add a vcpu,
> 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.
> 
> Signed-off-by: Chen Fan <chen.fan.fnst@cn.fujitsu.com>

This is regression is caused by commit c643bed99.

> ---
>  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..487a808 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.
> + * @apic_realize: Callback for realizing apic.
>   * @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 (*apic_realize)(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..88c6028 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 cpu_apic_realize(CPUState *cpu, Error **errp)
> +{
> +    CPUClass *cc = CPU_GET_CLASS(cpu);
> +    if (cc->apic_realize != NULL) {
> +        (*cc->apic_realize)(cpu, errp);
> +    }
> +}
> +
>  static void cpu_common_realizefn(DeviceState *dev, Error **errp)
>  {
>      CPUState *cpu = CPU(dev);
>  
>      qemu_init_vcpu(cpu);
>  
> +    cpu_apic_realize(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..916d69e 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->apic_realize = 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;

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

[Andreas Färber]

Hi,

Am 25.07.2013 08:27, schrieb Chen Fan:
> When useing x86_64-softmmu --enable-kvm boot qemu, cpu-add command fails to add a vcpu,
> 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.
> 
> 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(-)

First, please CC the maintainer - in this case me.

The referenced commit is this one:
http://git.qemu.org/?p=qemu.git;a=commit;h=c643bed99

Sorry for regressing.

> diff --git a/include/qom/cpu.h b/include/qom/cpu.h
> index daf1835..487a808 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.
> + * @apic_realize: Callback for realizing apic.
>   * @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 (*apic_realize)(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..88c6028 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 cpu_apic_realize(CPUState *cpu, Error **errp)
> +{
> +    CPUClass *cc = CPU_GET_CLASS(cpu);
> +    if (cc->apic_realize != NULL) {
> +        (*cc->apic_realize)(cpu, errp);
> +    }
> +}
> +
>  static void cpu_common_realizefn(DeviceState *dev, Error **errp)
>  {
>      CPUState *cpu = CPU(dev);
>  
>      qemu_init_vcpu(cpu);
>  
> +    cpu_apic_realize(cpu, errp);
> +    if (error_is_set(errp)) {
> +        return;
> +    }
> +
>      if (dev->hotplugged) {
>          cpu_synchronize_post_init(cpu);
>          notifier_list_notify(&cpu_added_notifiers, dev);

This is bogus since APIC is an x86 thing. It should stay in
target-i386/cpu.c. We could generalize it as a post-vCPU-init hook, but
I don't think that's needed here:

> diff --git a/target-i386/cpu.c b/target-i386/cpu.c
> index cd350cb..916d69e 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);

I think all that really needs to happen is to move the above four lines
to below xcc->parent_realize(). I doubt that a hot-add notifier is going
to fiddle with the APIC - we're just incrementing the number of CPUs in
the RTC today.

Can you send a v2 doing so please? Thanks.

Regards,
Andreas

> @@ -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->apic_realize = 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;
> 


-- 
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg

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

[chenfan]

On Thu, 2013-07-25 at 10:39 +0200, Andreas Färber wrote:
> Hi,
> 
> Am 25.07.2013 08:27, schrieb Chen Fan:
> > When useing x86_64-softmmu --enable-kvm boot qemu, cpu-add command fails to add a vcpu,
> > 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.
> > 
> > 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(-)
> 
> First, please CC the maintainer - in this case me.
> 
Ok.

> The referenced commit is this one:
> http://git.qemu.org/?p=qemu.git;a=commit;h=c643bed99
> 
> Sorry for regressing.
> 
> > diff --git a/include/qom/cpu.h b/include/qom/cpu.h
> > index daf1835..487a808 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.
> > + * @apic_realize: Callback for realizing apic.
> >   * @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 (*apic_realize)(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..88c6028 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 cpu_apic_realize(CPUState *cpu, Error **errp)
> > +{
> > +    CPUClass *cc = CPU_GET_CLASS(cpu);
> > +    if (cc->apic_realize != NULL) {
> > +        (*cc->apic_realize)(cpu, errp);
> > +    }
> > +}
> > +
> >  static void cpu_common_realizefn(DeviceState *dev, Error **errp)
> >  {
> >      CPUState *cpu = CPU(dev);
> >  
> >      qemu_init_vcpu(cpu);
> >  
> > +    cpu_apic_realize(cpu, errp);
> > +    if (error_is_set(errp)) {
> > +        return;
> > +    }
> > +
> >      if (dev->hotplugged) {
> >          cpu_synchronize_post_init(cpu);
> >          notifier_list_notify(&cpu_added_notifiers, dev);
> 
> This is bogus since APIC is an x86 thing. It should stay in
> target-i386/cpu.c. We could generalize it as a post-vCPU-init hook, but
> I don't think that's needed here:
> 
> > diff --git a/target-i386/cpu.c b/target-i386/cpu.c
> > index cd350cb..916d69e 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);
> 
> I think all that really needs to happen is to move the above four lines
> to below xcc->parent_realize(). I doubt that a hot-add notifier is going
> to fiddle with the APIC - we're just incrementing the number of CPUs in
> the RTC today.
> 
> Can you send a v2 doing so please? Thanks.
> 
I have already done so, unfortunately，when cpu-add, showing "smpboot:
CPU1: Not responding."
Maybe we need to use the post-vCPU-init hook to do this.

Thanks,
Chen

> Regards,
> Andreas
> 
> > @@ -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->apic_realize = 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;
> > 
> 
> 

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

[Igor Mammedov]

On Thu, 25 Jul 2013 10:39:54 +0200
Andreas Färber <afaerber@suse.de> wrote:

> Hi,
> 
> Am 25.07.2013 08:27, schrieb Chen Fan:
> > When useing x86_64-softmmu --enable-kvm boot qemu, cpu-add command fails to add a vcpu,
> > 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.
> > 
> > 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(-)
> 
> First, please CC the maintainer - in this case me.
> 
> The referenced commit is this one:
> http://git.qemu.org/?p=qemu.git;a=commit;h=c643bed99
> 
> Sorry for regressing.
> 
> > diff --git a/include/qom/cpu.h b/include/qom/cpu.h
> > index daf1835..487a808 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.
> > + * @apic_realize: Callback for realizing apic.
> >   * @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 (*apic_realize)(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..88c6028 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 cpu_apic_realize(CPUState *cpu, Error **errp)
> > +{
> > +    CPUClass *cc = CPU_GET_CLASS(cpu);
> > +    if (cc->apic_realize != NULL) {
> > +        (*cc->apic_realize)(cpu, errp);
> > +    }
> > +}
> > +
> >  static void cpu_common_realizefn(DeviceState *dev, Error **errp)
> >  {
> >      CPUState *cpu = CPU(dev);
> >  
> >      qemu_init_vcpu(cpu);
> >  
> > +    cpu_apic_realize(cpu, errp);
> > +    if (error_is_set(errp)) {
> > +        return;
> > +    }
> > +
> >      if (dev->hotplugged) {
> >          cpu_synchronize_post_init(cpu);
> >          notifier_list_notify(&cpu_added_notifiers, dev);
> 
> This is bogus since APIC is an x86 thing. It should stay in
> target-i386/cpu.c. We could generalize it as a post-vCPU-init hook, but
> I don't think that's needed here:
> 
> > diff --git a/target-i386/cpu.c b/target-i386/cpu.c
> > index cd350cb..916d69e 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);
> 
> I think all that really needs to happen is to move the above four lines
> to below xcc->parent_realize(). I doubt that a hot-add notifier is going
> to fiddle with the APIC - we're just incrementing the number of CPUs in
> the RTC today.
that would push uninitialized apic state into KVM
cpu_common_realizefn
  -> cpu_synchronize_post_init
    -> kvm_cpu_synchronize_post_init
      -> kvm_arch_put_registers
        -> kvm_put_apic
          -> kvm_put_apic_state
               kvm_vcpu_ioctl(CPU(cpu), KVM_SET_LAPIC, &kapic);

which becomes initialized only after x86_cpu_apic_realize().

It works for startup CPUs only because system wide reset
which calls cpu_synchronize_post_reset->...->kvm_put_apic_state()
and qemu_system_reset->...->apic_reset_common->kvm_apic_vapic_base_update()


in hotplug case flow was:
 x86_cpu_apic_realize->...->device_set_realized->realize()
                                               ->device_reset()
                                                 ->apic_reset_common->kvm_apic_vapic_base_update()
 cpu_common_realizefn
  -> cpu_synchronize_post_init
    -> kvm_cpu_synchronize_post_init
      -> kvm_arch_put_registers
        -> kvm_put_apic
                               
> Can you send a v2 doing so please? Thanks.
> 
> Regards,
> Andreas
> 
> > @@ -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->apic_realize = 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;
> > 
> 
>