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

login
register
mail settings
Submitter Igor Mammedov
Date Oct. 9, 2012, 5:17 p.m.
Message ID <1349803038-30502-1-git-send-email-imammedo@redhat.com>
Download mbox | patch
Permalink /patch/190381/
State New
Headers show

Comments

Igor Mammedov - Oct. 9, 2012, 5:17 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>

---
v2:
 * init APIC mapping at cpu level, due to Peter's objection to putting
   it into APIC's initfn and Jan's suggestion to do it inside cpu.
v3:
 * create APIC at realize time
 * rebased on top of current qemu tree
 * whitespace fix
 * ifdef only body of x86_cpu_apic_init()
v4:
 * put hw/xen.h under #ifndef CONFIG_USER_ONLY
 * check for error after x86_cpu_apic_init()
 * remove unneeded 'return'
v5:
 * use qdev_try_create() instead of qdev_create() to avoid abort at
   realize time. spotted-by: Andreas Färber <afaerber@suse.de>
---
 hw/pc.c           | 56 +++++-----------------------------------------------
 target-i386/cpu.c | 59 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 64 insertions(+), 51 deletions(-)
Andreas Färber - Oct. 10, 2012, 10:01 a.m.
Am 09.10.2012 19:17, 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>

I am happy with this latest round of fixes to ad-hoc feedback from my
CPU remodeling perspective (as an interim solution), and I depend on
this for my APIC changes (code is being moved around here).

> +    if (env->apic_state == NULL) {
> +        error_set(errp, QERR_DEVICE_INIT_FAILED, apic_type);
> +        return;
> +    }
[...]
> +    if (qdev_init(env->apic_state)) {
> +        error_set(errp, QERR_DEVICE_INIT_FAILED,
> +                  object_get_typename(OBJECT(env->apic_state)));
> +        return;
> +    }

Luiz, could you please take a look whether I can apply this to my
qom-cpu tree now or whether error_set() should be replaced by
error_setg() or something? Contents-wise these are error-checking
creation and initialization of a child (APIC) device/object.

Thanks,
Andreas
Igor Mammedov - Oct. 10, 2012, 12:49 p.m.
On Wed, 10 Oct 2012 12:01:29 +0200
Andreas Färber <afaerber@suse.de> wrote:

> Am 09.10.2012 19:17, 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>
> 
> I am happy with this latest round of fixes to ad-hoc feedback from my
> CPU remodeling perspective (as an interim solution), and I depend on
> this for my APIC changes (code is being moved around here).
> 
> > +    if (env->apic_state == NULL) {
> > +        error_set(errp, QERR_DEVICE_INIT_FAILED, apic_type);
This is equivalent to error_setg(errp, "Device '%s' could not be
initialized",...). I'm not sure if repeating the same string in many places
is a better than using an existing macro. 

> > +        return;
> > +    }
> [...]
> > +    if (qdev_init(env->apic_state)) {
> > +        error_set(errp, QERR_DEVICE_INIT_FAILED,u
> > +                  object_get_typename(OBJECT(env->apic_state)));
> > +        return;
> > +    }
> 
> Luiz, could you please take a look whether I can apply this to my
> qom-cpu tree now or whether error_set() should be replaced by
> error_setg() or something? Contents-wise these are error-checking
> creation and initialization of a child (APIC) device/object.

> 
> Thanks,
> Andreas
>
Andreas Färber - Oct. 10, 2012, 12:53 p.m.
Am 10.10.2012 14:49, schrieb Igor Mammedov:
> On Wed, 10 Oct 2012 12:01:29 +0200
> Andreas Färber <afaerber@suse.de> wrote:
> 
>> Am 09.10.2012 19:17, 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>
>>
>> I am happy with this latest round of fixes to ad-hoc feedback from my
>> CPU remodeling perspective (as an interim solution), and I depend on
>> this for my APIC changes (code is being moved around here).
>>
>>> +    if (env->apic_state == NULL) {
>>> +        error_set(errp, QERR_DEVICE_INIT_FAILED, apic_type);
> This is equivalent to error_setg(errp, "Device '%s' could not be
> initialized",...). I'm not sure if repeating the same string in many places
> is a better than using an existing macro. 

I thought their intention was to have specialized rather than standard
messages, so maybe "An APIC of type '%s' could not be created." and "The
APIC of type '%s' could not be initialized."?

Andreas

> 
>>> +        return;
>>> +    }
>> [...]
>>> +    if (qdev_init(env->apic_state)) {
>>> +        error_set(errp, QERR_DEVICE_INIT_FAILED,u
>>> +                  object_get_typename(OBJECT(env->apic_state)));
>>> +        return;
>>> +    }
>>
>> Luiz, could you please take a look whether I can apply this to my
>> qom-cpu tree now or whether error_set() should be replaced by
>> error_setg() or something? Contents-wise these are error-checking
>> creation and initialization of a child (APIC) device/object.
> 
>>
>> Thanks,
>> Andreas
>>
>
Luiz Capitulino - Oct. 10, 2012, 4:47 p.m.
On Wed, 10 Oct 2012 12:01:29 +0200
Andreas Färber <afaerber@suse.de> wrote:

> Am 09.10.2012 19:17, 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>
> 
> I am happy with this latest round of fixes to ad-hoc feedback from my
> CPU remodeling perspective (as an interim solution), and I depend on
> this for my APIC changes (code is being moved around here).
> 
> > +    if (env->apic_state == NULL) {
> > +        error_set(errp, QERR_DEVICE_INIT_FAILED, apic_type);
> > +        return;
> > +    }
> [...]
> > +    if (qdev_init(env->apic_state)) {
> > +        error_set(errp, QERR_DEVICE_INIT_FAILED,
> > +                  object_get_typename(OBJECT(env->apic_state)));
> > +        return;
> > +    }
> 
> Luiz, could you please take a look whether I can apply this to my
> qom-cpu tree now or whether error_set() should be replaced by
> error_setg() or something? Contents-wise these are error-checking
> creation and initialization of a child (APIC) device/object.

I only saw this after I replied to the last email I was copied in
this thread.

Is there anything else I should clarify?

Basically, we should not use QERR_* macros in new code. The only exception
are the ErrorClass errors, defined in qapi-schema.json (except GenericError).

If you call qdev_init() or whatever function several times and use the
same error message in all calls, then you should change qdev_init()
to take an Error argument.

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..f55ee35 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,14 +1876,67 @@  static void mce_init(X86CPU *cpu)
     }
 }
 
+#define MSI_ADDR_BASE 0xfee00000
+
+static void x86_cpu_apic_init(X86CPU *cpu, Error **errp)
+{
+#ifndef CONFIG_USER_ONLY
+    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_set(errp, QERR_DEVICE_INIT_FAILED, 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_set(errp, QERR_DEVICE_INIT_FAILED,
+                  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);
+    CPUX86State *env = &cpu->env;
 
 #ifndef CONFIG_USER_ONLY
     qemu_register_reset(x86_cpu_machine_reset_cb, cpu);
 #endif
 
+    if (env->cpuid_features & CPUID_APIC || smp_cpus > 1) {
+        x86_cpu_apic_init(cpu, errp);
+    }
+
+    if (error_is_set(errp)) {
+        return;
+    }
+
     mce_init(cpu);
     qemu_init_vcpu(&cpu->env);
     cpu_reset(CPU(cpu));