Patchwork [v5] target-i386: initialize APIC at CPU level

login
register
mail settings
Submitter Igor Mammedov
Date Oct. 13, 2012, 8:35 p.m.
Message ID <1350160539-22293-1-git-send-email-imammedo@redhat.com>
Download mbox | patch
Permalink /patch/191311/
State New
Headers show

Comments

Igor Mammedov - Oct. 13, 2012, 8:35 p.m.
(L)APIC is a part of cpu [1] so move APIC initialization inside of
x86_cpu object. Since cpu_model and override flags currently specify
whether APIC should be created or not, APIC creation&initialization is
moved into x86_cpu_apic_init() which is called from x86_cpu_realize().

[1] - all x86 cpus have integrated APIC if we overlook existence of i486,
and it's more convenient to model after majority of them.

Signed-off-by: Igor Mammedov <imammedo@redhat.com>
---
  v5: fix *-user target build, smp_cpus is defined for softmmu only

git tree for testing:
  https://github.com/imammedo/qemu/tree/apic_in_cpu.v5
---
 hw/pc.c           | 56 +++++-------------------------------------------------
 target-i386/cpu.c | 57 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 62 insertions(+), 51 deletions(-)
Andreas Färber - Oct. 14, 2012, 4:09 a.m.
Am 13.10.2012 22:35, schrieb Igor Mammedov:
> (L)APIC is a part of cpu [1] so move APIC initialization inside of
> x86_cpu object. Since cpu_model and override flags currently specify
> whether APIC should be created or not, APIC creation&initialization is
> moved into x86_cpu_apic_init() which is called from x86_cpu_realize().
> 
> [1] - all x86 cpus have integrated APIC if we overlook existence of i486,
> and it's more convenient to model after majority of them.
> 
> Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> ---
>   v5: fix *-user target build, smp_cpus is defined for softmmu only

I do not run into any build issue with v4. Is it a runtime issue?

Andreas
Igor Mammedov - Oct. 15, 2012, 11:54 a.m.
On Sun, 14 Oct 2012 06:09:56 +0200
Andreas Färber <afaerber@suse.de> wrote:

> Am 13.10.2012 22:35, schrieb Igor Mammedov:
> > (L)APIC is a part of cpu [1] so move APIC initialization inside of
> > x86_cpu object. Since cpu_model and override flags currently specify
> > whether APIC should be created or not, APIC creation&initialization is
> > moved into x86_cpu_apic_init() which is called from x86_cpu_realize().
> > 
> > [1] - all x86 cpus have integrated APIC if we overlook existence of i486,
> > and it's more convenient to model after majority of them.
> > 
> > Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> > ---
> >   v5: fix *-user target build, smp_cpus is defined for softmmu only
> 
> I do not run into any build issue with v4. Is it a runtime issue?
> 
> Andreas
> 
I've run into this trying to build your latest CPUstate series with
following configure options:
  './configure' '--enable-debug'  '--target-list=x86_64-softmmu,x86_64-linux-user'

Anyway it's better not to build in any APIC checks in user target since it
doesn't need it at all.

I'm sorry for not noticing error earlier at v4 build time.
Could you re-apply it, please?
Andreas Färber - Oct. 15, 2012, 12:58 p.m.
Am 15.10.2012 13:54, schrieb Igor Mammedov:
> On Sun, 14 Oct 2012 06:09:56 +0200
> Andreas Färber <afaerber@suse.de> wrote:
> 
>> Am 13.10.2012 22:35, schrieb Igor Mammedov:
>>> (L)APIC is a part of cpu [1] so move APIC initialization inside of
>>> x86_cpu object. Since cpu_model and override flags currently specify
>>> whether APIC should be created or not, APIC creation&initialization is
>>> moved into x86_cpu_apic_init() which is called from x86_cpu_realize().
>>>
>>> [1] - all x86 cpus have integrated APIC if we overlook existence of i486,
>>> and it's more convenient to model after majority of them.
>>>
>>> Signed-off-by: Igor Mammedov <imammedo@redhat.com>
>>> ---
>>>   v5: fix *-user target build, smp_cpus is defined for softmmu only
>>
>> I do not run into any build issue with v4. Is it a runtime issue?
>>
>> Andreas
>>
> I've run into this trying to build your latest CPUstate series with
> following configure options:
>   './configure' '--enable-debug'  '--target-list=x86_64-softmmu,x86_64-linux-user'

--enable-debug was the missing puzzle piece, now I get the undefined
reference warning, too. :)

> Anyway it's better not to build in any APIC checks in user target since it
> doesn't need it at all.
> 
> I'm sorry for not noticing error earlier at v4 build time.
> Could you re-apply it, please?

Sure, the patch itself is indeed better. Now that I've been able to
reproduce, I've exchanged v4 against v5 in on qom-cpu queue:
https://github.com/afaerber/qemu-cpu/commits/qom-cpu

Thanks for noticing,
Andreas
Igor Mammedov - Oct. 15, 2012, 1:05 p.m.
On Mon, 15 Oct 2012 14:58:40 +0200
Andreas Färber <afaerber@suse.de> wrote:

> Am 15.10.2012 13:54, schrieb Igor Mammedov:
> > On Sun, 14 Oct 2012 06:09:56 +0200
> > Andreas Färber <afaerber@suse.de> wrote:
> > 
> >> Am 13.10.2012 22:35, schrieb Igor Mammedov:
> >>> (L)APIC is a part of cpu [1] so move APIC initialization inside of
> >>> x86_cpu object. Since cpu_model and override flags currently specify
> >>> whether APIC should be created or not, APIC creation&initialization is
> >>> moved into x86_cpu_apic_init() which is called from x86_cpu_realize().
> >>>
> >>> [1] - all x86 cpus have integrated APIC if we overlook existence of
> >>> i486, and it's more convenient to model after majority of them.
> >>>
> >>> Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> >>> ---
> >>>   v5: fix *-user target build, smp_cpus is defined for softmmu only
> >>
> >> I do not run into any build issue with v4. Is it a runtime issue?
> >>
> >> Andreas
> >>
> > I've run into this trying to build your latest CPUstate series with
> > following configure options:
> >   './configure' '--enable-debug'
> > '--target-list=x86_64-softmmu,x86_64-linux-user'
> 
> --enable-debug was the missing puzzle piece, now I get the undefined
> reference warning, too. :)
> 
> > Anyway it's better not to build in any APIC checks in user target since it
> > doesn't need it at all.
> > 
> > I'm sorry for not noticing error earlier at v4 build time.
> > Could you re-apply it, please?
> 
> Sure, the patch itself is indeed better. Now that I've been able to
> reproduce, I've exchanged v4 against v5 in on qom-cpu queue:
> https://github.com/afaerber/qemu-cpu/commits/qom-cpu
Thanks.

> 
> Thanks for noticing,
> Andreas
>

Patch

diff --git a/hw/pc.c b/hw/pc.c
index 6c0722d..aa5abe0 100644
--- a/hw/pc.c
+++ b/hw/pc.c
@@ -70,8 +70,6 @@ 
 #define FW_CFG_E820_TABLE (FW_CFG_ARCH_LOCAL + 3)
 #define FW_CFG_HPET (FW_CFG_ARCH_LOCAL + 4)
 
-#define MSI_ADDR_BASE 0xfee00000
-
 #define E820_NR_ENTRIES		16
 
 struct e820_entry {
@@ -845,35 +843,6 @@  DeviceState *cpu_get_current_apic(void)
     }
 }
 
-static DeviceState *apic_init(void *env, uint8_t apic_id)
-{
-    DeviceState *dev;
-    static int apic_mapped;
-
-    if (kvm_irqchip_in_kernel()) {
-        dev = qdev_create(NULL, "kvm-apic");
-    } else if (xen_enabled()) {
-        dev = qdev_create(NULL, "xen-apic");
-    } else {
-        dev = qdev_create(NULL, "apic");
-    }
-
-    qdev_prop_set_uint8(dev, "id", apic_id);
-    qdev_prop_set_ptr(dev, "cpu_env", env);
-    qdev_init_nofail(dev);
-
-    /* XXX: mapping more APICs at the same memory location */
-    if (apic_mapped == 0) {
-        /* NOTE: the APIC is directly connected to the CPU - it is not
-           on the global memory bus. */
-        /* XXX: what if the base changes? */
-        sysbus_mmio_map(sysbus_from_qdev(dev), 0, MSI_ADDR_BASE);
-        apic_mapped = 1;
-    }
-
-    return dev;
-}
-
 void pc_acpi_smi_interrupt(void *opaque, int irq, int level)
 {
     CPUX86State *s = opaque;
@@ -883,24 +852,6 @@  void pc_acpi_smi_interrupt(void *opaque, int irq, int level)
     }
 }
 
-static X86CPU *pc_new_cpu(const char *cpu_model)
-{
-    X86CPU *cpu;
-    CPUX86State *env;
-
-    cpu = cpu_x86_init(cpu_model);
-    if (cpu == NULL) {
-        fprintf(stderr, "Unable to find x86 CPU definition\n");
-        exit(1);
-    }
-    env = &cpu->env;
-    if ((env->cpuid_features & CPUID_APIC) || smp_cpus > 1) {
-        env->apic_state = apic_init(env, env->cpuid_apic_id);
-    }
-    cpu_reset(CPU(cpu));
-    return cpu;
-}
-
 void pc_cpus_init(const char *cpu_model)
 {
     int i;
@@ -914,8 +865,11 @@  void pc_cpus_init(const char *cpu_model)
 #endif
     }
 
-    for(i = 0; i < smp_cpus; i++) {
-        pc_new_cpu(cpu_model);
+    for (i = 0; i < smp_cpus; i++) {
+        if (!cpu_x86_init(cpu_model)) {
+            fprintf(stderr, "Unable to find x86 CPU definition\n");
+            exit(1);
+        }
     }
 }
 
diff --git a/target-i386/cpu.c b/target-i386/cpu.c
index f3708e6..b5cf37d 100644
--- a/target-i386/cpu.c
+++ b/target-i386/cpu.c
@@ -37,6 +37,12 @@ 
 #include <linux/kvm_para.h>
 #endif
 
+#include "sysemu.h"
+#ifndef CONFIG_USER_ONLY
+#include "hw/xen.h"
+#include "hw/sysbus.h"
+#endif
+
 /* feature flags taken from "Intel Processor Identification and the CPUID
  * Instruction" and AMD's "CPUID Specification".  In cases of disagreement
  * between feature naming conventions, aliases may be added.
@@ -1870,12 +1876,63 @@  static void mce_init(X86CPU *cpu)
     }
 }
 
+#define MSI_ADDR_BASE 0xfee00000
+
+#ifndef CONFIG_USER_ONLY
+static void x86_cpu_apic_init(X86CPU *cpu, Error **errp)
+{
+    static int apic_mapped;
+    CPUX86State *env = &cpu->env;
+    const char *apic_type = "apic";
+
+    if (kvm_irqchip_in_kernel()) {
+        apic_type = "kvm-apic";
+    } else if (xen_enabled()) {
+        apic_type = "xen-apic";
+    }
+
+    env->apic_state = qdev_try_create(NULL, apic_type);
+    if (env->apic_state == NULL) {
+        error_setg(errp, "APIC device '%s' could not be created", apic_type);
+        return;
+    }
+
+    object_property_add_child(OBJECT(cpu), "apic",
+                              OBJECT(env->apic_state), NULL);
+    qdev_prop_set_uint8(env->apic_state, "id", env->cpuid_apic_id);
+    /* TODO: convert to link<> */
+    qdev_prop_set_ptr(env->apic_state, "cpu_env", env);
+
+    if (qdev_init(env->apic_state)) {
+        error_setg(errp, "APIC device '%s' could not be initialized",
+                   object_get_typename(OBJECT(env->apic_state)));
+        return;
+    }
+
+    /* XXX: mapping more APICs at the same memory location */
+    if (apic_mapped == 0) {
+        /* NOTE: the APIC is directly connected to the CPU - it is not
+           on the global memory bus. */
+        /* XXX: what if the base changes? */
+        sysbus_mmio_map(sysbus_from_qdev(env->apic_state), 0, MSI_ADDR_BASE);
+        apic_mapped = 1;
+    }
+}
+#endif
+
 void x86_cpu_realize(Object *obj, Error **errp)
 {
     X86CPU *cpu = X86_CPU(obj);
 
 #ifndef CONFIG_USER_ONLY
     qemu_register_reset(x86_cpu_machine_reset_cb, cpu);
+
+    if (cpu->env.cpuid_features & CPUID_APIC || smp_cpus > 1) {
+        x86_cpu_apic_init(cpu, errp);
+        if (error_is_set(errp)) {
+            return;
+        }
+    }
 #endif
 
     mce_init(cpu);