| Message ID | 1349803038-30502-1-git-send-email-imammedo@redhat.com |
|---|---|
| State | New |
| Headers | show |
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
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 >
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 >> >
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.
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));
(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(-)