| Message ID | 1338329426-28118-12-git-send-email-imammedo@redhat.com |
|---|---|
| State | New |
| Headers | show |
On 2012-05-30 00:10, Igor Mammedov wrote: > (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 is moved into > cpu_model property setter. And APIC 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. > > 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. > > Signed-off-by: Igor Mammedov <imammedo@redhat.com> > --- > hw/pc.c | 44 ------------------------------------- > target-i386/cpu.c | 62 +++++++++++++++++++++++++++++++++++++++++++++++++++++ > 2 files changed, 62 insertions(+), 44 deletions(-) > > diff --git a/hw/pc.c b/hw/pc.c > index d178801..986f119 100644 > --- a/hw/pc.c > +++ b/hw/pc.c > @@ -42,7 +42,6 @@ > #include "sysbus.h" > #include "sysemu.h" > #include "kvm.h" > -#include "xen.h" > #include "blockdev.h" > #include "ui/qemu-spice.h" > #include "memory.h" > @@ -70,8 +69,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 { > @@ -879,42 +876,6 @@ DeviceState *cpu_get_current_apic(void) > } > } > > -static DeviceState *apic_init(void *env, uint8_t apic_id) > -{ > - DeviceState *dev; > - Error *error = NULL; > - 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); > - object_property_set_link(OBJECT(dev), OBJECT(ENV_GET_CPU(env)), "cpu", > - &error); > - if (error_is_set(&error)) { > - qerror_report_err(error); > - error_free(error); > - exit(1); > - } > - 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; > @@ -933,17 +894,12 @@ static void pc_cpu_reset(void *opaque) > static X86CPU *pc_new_cpu(const char *cpu_model) > { > X86CPU *cpu; > - CPUX86State *env; > > cpu = cpu_x86_init(cpu_model); > if (cpu == NULL) { > exit(1); > } > - env = &cpu->env; > > - if ((env->cpuid_features & CPUID_APIC) || smp_cpus > 1) { > - env->apic_state = apic_init(env, env->cpuid_apic_id); > - } > qemu_register_reset(pc_cpu_reset, cpu); > pc_cpu_reset(cpu); > return cpu; > diff --git a/target-i386/cpu.c b/target-i386/cpu.c > index 2610d96..b649904 100644 > --- a/target-i386/cpu.c > +++ b/target-i386/cpu.c > @@ -23,6 +23,7 @@ > > #include "cpu.h" > #include "kvm.h" > +#include "hw/xen.h" > > #include "qemu-option.h" > #include "qemu-config.h" > @@ -31,6 +32,11 @@ > > #include "hyperv.h" > > +#include "sysemu.h" > +#ifndef CONFIG_USER_ONLY > +#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. > @@ -1749,13 +1755,69 @@ static void x86_set_cpu_model(Object *obj, const char *value, Error **errp) > if (cpu_x86_register(cpu, env->cpu_model_str) < 0) { > fprintf(stderr, "Unable to find x86 CPU definition\n"); > error_set(errp, QERR_INVALID_PARAMETER_COMBINATION); > + return; > + } > + > +#ifndef CONFIG_USER_ONLY > + if (((env->cpuid_features & CPUID_APIC) || smp_cpus > 1)) { > + if (kvm_irqchip_in_kernel()) { > + env->apic_state = qdev_create(NULL, "kvm-apic"); > + } else if (xen_enabled()) { > + env->apic_state = qdev_create(NULL, "xen-apic"); > + } else { > + env->apic_state = qdev_create(NULL, "apic"); > + } > + object_property_add_child(OBJECT(cpu), "apic", > + OBJECT(env->apic_state), NULL); > + > + qdev_prop_set_uint8(env->apic_state, "id", env->cpuid_apic_id); > + object_property_set_link(OBJECT(env->apic_state), OBJECT(cpu), "cpu", > + errp); > + if (error_is_set(errp)) { > + return; > + } > + } > +#endif > +} > + > +#ifndef CONFIG_USER_ONLY > +#define MSI_ADDR_BASE 0xfee00000 > + > +static void x86_cpu_apic_init(X86CPU *cpu, Error **errp) > +{ > + static int apic_mapped; > + CPUX86State *env = &cpu->env; > + > + if (env->apic_state == NULL) { > + return; > + } > + > + 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); > > + x86_cpu_apic_init(cpu, errp); > + if (error_is_set(errp)) { > + return; > + } > + I guess you are lacking #ifndef CONFIG_USER_ONLY here already (for the sake of bisectability). Or, likely better, let x86_cpu_apic_init do nothing for usermode emulation. Jan
On 05/30/2012 12:41 PM, Jan Kiszka wrote: > On 2012-05-30 00:10, Igor Mammedov wrote: >> (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 is moved into >> cpu_model property setter. And APIC 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. >> >> 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. >> >> Signed-off-by: Igor Mammedov<imammedo@redhat.com> >> --- >> hw/pc.c | 44 ------------------------------------- >> target-i386/cpu.c | 62 +++++++++++++++++++++++++++++++++++++++++++++++++++++ >> 2 files changed, 62 insertions(+), 44 deletions(-) >> >> diff --git a/hw/pc.c b/hw/pc.c >> index d178801..986f119 100644 >> --- a/hw/pc.c >> +++ b/hw/pc.c >> @@ -42,7 +42,6 @@ >> #include "sysbus.h" >> #include "sysemu.h" >> #include "kvm.h" >> -#include "xen.h" >> #include "blockdev.h" >> #include "ui/qemu-spice.h" >> #include "memory.h" >> @@ -70,8 +69,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 { >> @@ -879,42 +876,6 @@ DeviceState *cpu_get_current_apic(void) >> } >> } >> >> -static DeviceState *apic_init(void *env, uint8_t apic_id) >> -{ >> - DeviceState *dev; >> - Error *error = NULL; >> - 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); >> - object_property_set_link(OBJECT(dev), OBJECT(ENV_GET_CPU(env)), "cpu", >> -&error); >> - if (error_is_set(&error)) { >> - qerror_report_err(error); >> - error_free(error); >> - exit(1); >> - } >> - 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; >> @@ -933,17 +894,12 @@ static void pc_cpu_reset(void *opaque) >> static X86CPU *pc_new_cpu(const char *cpu_model) >> { >> X86CPU *cpu; >> - CPUX86State *env; >> >> cpu = cpu_x86_init(cpu_model); >> if (cpu == NULL) { >> exit(1); >> } >> - env =&cpu->env; >> >> - if ((env->cpuid_features& CPUID_APIC) || smp_cpus> 1) { >> - env->apic_state = apic_init(env, env->cpuid_apic_id); >> - } >> qemu_register_reset(pc_cpu_reset, cpu); >> pc_cpu_reset(cpu); >> return cpu; >> diff --git a/target-i386/cpu.c b/target-i386/cpu.c >> index 2610d96..b649904 100644 >> --- a/target-i386/cpu.c >> +++ b/target-i386/cpu.c >> @@ -23,6 +23,7 @@ >> >> #include "cpu.h" >> #include "kvm.h" >> +#include "hw/xen.h" >> >> #include "qemu-option.h" >> #include "qemu-config.h" >> @@ -31,6 +32,11 @@ >> >> #include "hyperv.h" >> >> +#include "sysemu.h" >> +#ifndef CONFIG_USER_ONLY >> +#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. >> @@ -1749,13 +1755,69 @@ static void x86_set_cpu_model(Object *obj, const char *value, Error **errp) >> if (cpu_x86_register(cpu, env->cpu_model_str)< 0) { >> fprintf(stderr, "Unable to find x86 CPU definition\n"); >> error_set(errp, QERR_INVALID_PARAMETER_COMBINATION); >> + return; >> + } >> + >> +#ifndef CONFIG_USER_ONLY >> + if (((env->cpuid_features& CPUID_APIC) || smp_cpus> 1)) { >> + if (kvm_irqchip_in_kernel()) { >> + env->apic_state = qdev_create(NULL, "kvm-apic"); >> + } else if (xen_enabled()) { >> + env->apic_state = qdev_create(NULL, "xen-apic"); >> + } else { >> + env->apic_state = qdev_create(NULL, "apic"); >> + } >> + object_property_add_child(OBJECT(cpu), "apic", >> + OBJECT(env->apic_state), NULL); >> + >> + qdev_prop_set_uint8(env->apic_state, "id", env->cpuid_apic_id); >> + object_property_set_link(OBJECT(env->apic_state), OBJECT(cpu), "cpu", >> + errp); >> + if (error_is_set(errp)) { >> + return; >> + } >> + } >> +#endif >> +} >> + >> +#ifndef CONFIG_USER_ONLY >> +#define MSI_ADDR_BASE 0xfee00000 >> + >> +static void x86_cpu_apic_init(X86CPU *cpu, Error **errp) >> +{ >> + static int apic_mapped; >> + CPUX86State *env =&cpu->env; >> + >> + if (env->apic_state == NULL) { >> + return; >> + } >> + >> + 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); >> >> + x86_cpu_apic_init(cpu, errp); >> + if (error_is_set(errp)) { >> + return; >> + } >> + > > I guess you are lacking #ifndef CONFIG_USER_ONLY here already (for the > sake of bisectability). Or, likely better, let x86_cpu_apic_init do > nothing for usermode emulation. initially x86_cpu_apic_init() was doing nothing for usermode, hence lack of #ifndef CONFIG_USER_ONLY, but then for patch 12/12 #ifndef CONFIG_USER_ONLY required here any way so I've removed usermode stub x86_cpu_apic_init() and squashed change in wrong patch :(. And since I need #ifndef in initfn anyway then probably there is no point in having x86_cpu_apic_init(), I'll move its body in initfn then. Thanks > > Jan >
On 2012-05-30 16:27, Igor Mammedov wrote: >>> + >>> +#ifndef CONFIG_USER_ONLY >>> +#define MSI_ADDR_BASE 0xfee00000 >>> + >>> +static void x86_cpu_apic_init(X86CPU *cpu, Error **errp) >>> +{ >>> + static int apic_mapped; >>> + CPUX86State *env =&cpu->env; >>> + >>> + if (env->apic_state == NULL) { >>> + return; >>> + } >>> + >>> + 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); >>> >>> + x86_cpu_apic_init(cpu, errp); >>> + if (error_is_set(errp)) { >>> + return; >>> + } >>> + >> >> I guess you are lacking #ifndef CONFIG_USER_ONLY here already (for the >> sake of bisectability). Or, likely better, let x86_cpu_apic_init do >> nothing for usermode emulation. > initially x86_cpu_apic_init() was doing nothing for usermode, hence lack of > #ifndef CONFIG_USER_ONLY, but then for patch 12/12 #ifndef CONFIG_USER_ONLY > required here any way so I've removed usermode stub x86_cpu_apic_init() and > squashed change in wrong patch :(. > > And since I need #ifndef in initfn anyway then probably there is no point > in having x86_cpu_apic_init(), I'll move its body in initfn then. I think a function is cleaner, and we have some other examples for this already in this context. Its body could easily be #ifdef'ed out (the other reason for #ifdef in the initfn are temporary, no?). Jan
On 05/30/2012 04:38 PM, Jan Kiszka wrote: > On 2012-05-30 16:27, Igor Mammedov wrote: >>>> + >>>> +#ifndef CONFIG_USER_ONLY >>>> +#define MSI_ADDR_BASE 0xfee00000 >>>> + >>>> +static void x86_cpu_apic_init(X86CPU *cpu, Error **errp) >>>> +{ >>>> + static int apic_mapped; >>>> + CPUX86State *env =&cpu->env; >>>> + >>>> + if (env->apic_state == NULL) { >>>> + return; >>>> + } >>>> + >>>> + 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); >>>> >>>> + x86_cpu_apic_init(cpu, errp); >>>> + if (error_is_set(errp)) { >>>> + return; >>>> + } >>>> + >>> >>> I guess you are lacking #ifndef CONFIG_USER_ONLY here already (for the >>> sake of bisectability). Or, likely better, let x86_cpu_apic_init do >>> nothing for usermode emulation. >> initially x86_cpu_apic_init() was doing nothing for usermode, hence lack of >> #ifndef CONFIG_USER_ONLY, but then for patch 12/12 #ifndef CONFIG_USER_ONLY >> required here any way so I've removed usermode stub x86_cpu_apic_init() and >> squashed change in wrong patch :(. >> >> And since I need #ifndef in initfn anyway then probably there is no point >> in having x86_cpu_apic_init(), I'll move its body in initfn then. > > I think a function is cleaner, and we have some other examples for this > already in this context. Its body could easily be #ifdef'ed out (the > other reason for #ifdef in the initfn are temporary, no?). Yes, is temporary. However if Peter could be persuaded not to object for putting mapping of APIC base into apic's initfn then x86_cpu_apic_init() would be temporary as well (qdev_init part goes away with realize property and apic base mapping could be moved into it's own property setter in apic object and default mapping set in its initfn). Andreas seems liked putting it there. Then overall code would look cleaner and with less ifdefs. > > Jan >
On 2012-05-30 16:52, Igor Mammedov wrote: > On 05/30/2012 04:38 PM, Jan Kiszka wrote: >> On 2012-05-30 16:27, Igor Mammedov wrote: >>>>> + >>>>> +#ifndef CONFIG_USER_ONLY >>>>> +#define MSI_ADDR_BASE 0xfee00000 >>>>> + >>>>> +static void x86_cpu_apic_init(X86CPU *cpu, Error **errp) >>>>> +{ >>>>> + static int apic_mapped; >>>>> + CPUX86State *env =&cpu->env; >>>>> + >>>>> + if (env->apic_state == NULL) { >>>>> + return; >>>>> + } >>>>> + >>>>> + 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); >>>>> >>>>> + x86_cpu_apic_init(cpu, errp); >>>>> + if (error_is_set(errp)) { >>>>> + return; >>>>> + } >>>>> + >>>> >>>> I guess you are lacking #ifndef CONFIG_USER_ONLY here already (for the >>>> sake of bisectability). Or, likely better, let x86_cpu_apic_init do >>>> nothing for usermode emulation. >>> initially x86_cpu_apic_init() was doing nothing for usermode, hence lack of >>> #ifndef CONFIG_USER_ONLY, but then for patch 12/12 #ifndef CONFIG_USER_ONLY >>> required here any way so I've removed usermode stub x86_cpu_apic_init() and >>> squashed change in wrong patch :(. >>> >>> And since I need #ifndef in initfn anyway then probably there is no point >>> in having x86_cpu_apic_init(), I'll move its body in initfn then. >> >> I think a function is cleaner, and we have some other examples for this >> already in this context. Its body could easily be #ifdef'ed out (the >> other reason for #ifdef in the initfn are temporary, no?). > Yes, is temporary. > However if Peter could be persuaded not to object for putting mapping of APIC base > into apic's initfn then x86_cpu_apic_init() would be temporary as well (qdev_init part > goes away with realize property and apic base mapping could be moved into it's > own property setter in apic object and default mapping set in its initfn). > Andreas seems liked putting it there. > Then overall code would look cleaner and with less ifdefs. "Unfortunately", the mapping belongs to the CPU because it actually performs it as we pointed out. Jan
On 05/30/2012 04:55 PM, Jan Kiszka wrote: > On 2012-05-30 16:52, Igor Mammedov wrote: >> On 05/30/2012 04:38 PM, Jan Kiszka wrote: >>> On 2012-05-30 16:27, Igor Mammedov wrote: >>>>>> + >>>>>> +#ifndef CONFIG_USER_ONLY >>>>>> +#define MSI_ADDR_BASE 0xfee00000 >>>>>> + >>>>>> +static void x86_cpu_apic_init(X86CPU *cpu, Error **errp) >>>>>> +{ >>>>>> + static int apic_mapped; >>>>>> + CPUX86State *env =&cpu->env; >>>>>> + >>>>>> + if (env->apic_state == NULL) { >>>>>> + return; >>>>>> + } >>>>>> + >>>>>> + 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); >>>>>> >>>>>> + x86_cpu_apic_init(cpu, errp); >>>>>> + if (error_is_set(errp)) { >>>>>> + return; >>>>>> + } >>>>>> + >>>>> >>>>> I guess you are lacking #ifndef CONFIG_USER_ONLY here already (for the >>>>> sake of bisectability). Or, likely better, let x86_cpu_apic_init do >>>>> nothing for usermode emulation. >>>> initially x86_cpu_apic_init() was doing nothing for usermode, hence lack of >>>> #ifndef CONFIG_USER_ONLY, but then for patch 12/12 #ifndef CONFIG_USER_ONLY >>>> required here any way so I've removed usermode stub x86_cpu_apic_init() and >>>> squashed change in wrong patch :(. >>>> >>>> And since I need #ifndef in initfn anyway then probably there is no point >>>> in having x86_cpu_apic_init(), I'll move its body in initfn then. >>> >>> I think a function is cleaner, and we have some other examples for this >>> already in this context. Its body could easily be #ifdef'ed out (the >>> other reason for #ifdef in the initfn are temporary, no?). >> Yes, is temporary. >> However if Peter could be persuaded not to object for putting mapping of APIC base >> into apic's initfn then x86_cpu_apic_init() would be temporary as well (qdev_init part >> goes away with realize property and apic base mapping could be moved into it's >> own property setter in apic object and default mapping set in its initfn). >> Andreas seems liked putting it there. >> Then overall code would look cleaner and with less ifdefs. > > "Unfortunately", the mapping belongs to the CPU because it actually > performs it as we pointed out. Intel SDM tells only about RE-mapping that could be done via msr, but I haven't found any mention about what part of CPU does default initial mapping (it might be APIC itself). Docs just state for external and internal APICs that they are mapped at default base right after power on or reset. :( Any way moving APIC mapping code in it's own setter inside apic_common object might be good idea on its own even if it's called from CPU to set initial mapping. PS: May be Intel guys could tell us how it's done in real hardware if it's not a secret. > > Jan >
diff --git a/hw/pc.c b/hw/pc.c index d178801..986f119 100644 --- a/hw/pc.c +++ b/hw/pc.c @@ -42,7 +42,6 @@ #include "sysbus.h" #include "sysemu.h" #include "kvm.h" -#include "xen.h" #include "blockdev.h" #include "ui/qemu-spice.h" #include "memory.h" @@ -70,8 +69,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 { @@ -879,42 +876,6 @@ DeviceState *cpu_get_current_apic(void) } } -static DeviceState *apic_init(void *env, uint8_t apic_id) -{ - DeviceState *dev; - Error *error = NULL; - 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); - object_property_set_link(OBJECT(dev), OBJECT(ENV_GET_CPU(env)), "cpu", - &error); - if (error_is_set(&error)) { - qerror_report_err(error); - error_free(error); - exit(1); - } - 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; @@ -933,17 +894,12 @@ static void pc_cpu_reset(void *opaque) static X86CPU *pc_new_cpu(const char *cpu_model) { X86CPU *cpu; - CPUX86State *env; cpu = cpu_x86_init(cpu_model); if (cpu == NULL) { exit(1); } - env = &cpu->env; - if ((env->cpuid_features & CPUID_APIC) || smp_cpus > 1) { - env->apic_state = apic_init(env, env->cpuid_apic_id); - } qemu_register_reset(pc_cpu_reset, cpu); pc_cpu_reset(cpu); return cpu; diff --git a/target-i386/cpu.c b/target-i386/cpu.c index 2610d96..b649904 100644 --- a/target-i386/cpu.c +++ b/target-i386/cpu.c @@ -23,6 +23,7 @@ #include "cpu.h" #include "kvm.h" +#include "hw/xen.h" #include "qemu-option.h" #include "qemu-config.h" @@ -31,6 +32,11 @@ #include "hyperv.h" +#include "sysemu.h" +#ifndef CONFIG_USER_ONLY +#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. @@ -1749,13 +1755,69 @@ static void x86_set_cpu_model(Object *obj, const char *value, Error **errp) if (cpu_x86_register(cpu, env->cpu_model_str) < 0) { fprintf(stderr, "Unable to find x86 CPU definition\n"); error_set(errp, QERR_INVALID_PARAMETER_COMBINATION); + return; + } + +#ifndef CONFIG_USER_ONLY + if (((env->cpuid_features & CPUID_APIC) || smp_cpus > 1)) { + if (kvm_irqchip_in_kernel()) { + env->apic_state = qdev_create(NULL, "kvm-apic"); + } else if (xen_enabled()) { + env->apic_state = qdev_create(NULL, "xen-apic"); + } else { + env->apic_state = qdev_create(NULL, "apic"); + } + object_property_add_child(OBJECT(cpu), "apic", + OBJECT(env->apic_state), NULL); + + qdev_prop_set_uint8(env->apic_state, "id", env->cpuid_apic_id); + object_property_set_link(OBJECT(env->apic_state), OBJECT(cpu), "cpu", + errp); + if (error_is_set(errp)) { + return; + } + } +#endif +} + +#ifndef CONFIG_USER_ONLY +#define MSI_ADDR_BASE 0xfee00000 + +static void x86_cpu_apic_init(X86CPU *cpu, Error **errp) +{ + static int apic_mapped; + CPUX86State *env = &cpu->env; + + if (env->apic_state == NULL) { + return; + } + + 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); + x86_cpu_apic_init(cpu, errp); + if (error_is_set(errp)) { + return; + } + mce_init(cpu); qemu_init_vcpu(&cpu->env); }
(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 is moved into cpu_model property setter. And APIC 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. 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. Signed-off-by: Igor Mammedov <imammedo@redhat.com> --- hw/pc.c | 44 ------------------------------------- target-i386/cpu.c | 62 +++++++++++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 62 insertions(+), 44 deletions(-)