[Top][All Lists]
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH qom-next 5/6] target-i386: make initialize CPU i
From: |
Andreas Färber |
Subject: |
Re: [Qemu-devel] [PATCH qom-next 5/6] target-i386: make initialize CPU in QOM way |
Date: |
Wed, 23 May 2012 23:27:49 +0200 |
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:12.0) Gecko/20120421 Thunderbird/12.0 |
Am 23.05.2012 18:39, schrieb Igor Mammedov:
> Make CPU creation/initialization consistent with QOM object
> behavior in this, by moving tcg and apic initialization from board
> level into CPU's initfn/realize calls and cpu_model property setter.
>
> Which makes CPU object self-sufficient in respect of creation/initialization
> and matches a typical object creation sequence, i.e.:
> - create CPU instance
> - set properties
> - realize object - (x86_cpu_realize will be converted into realize
> property setter, when it is implemented)
>
> v2:
> - fix moving of tcg_* initialization into cpu.c from helper.c
> spotted-by: <Jan Kiszka address@hidden>
> - make it compile/work on i386-linux-user target
>
> Signed-off-by: Igor Mammedov <address@hidden>
> ---
> hw/pc.c | 45 ++++------------------------
> target-i386/cpu.c | 81 ++++++++++++++++++++++++++++++++++++++++++++++++-
> target-i386/helper.c | 39 ------------------------
> 3 files changed, 85 insertions(+), 80 deletions(-)
>
> diff --git a/hw/pc.c b/hw/pc.c
> index 1ccfc6e..d7845ea 100644
> --- a/hw/pc.c
> +++ b/hw/pc.c
> @@ -911,30 +891,17 @@ static void pc_cpu_reset(void *opaque)
> cpu_reset(CPU(cpu));
> }
>
> -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;
> -}
> -
> void pc_cpus_init(const char *cpu_model)
> {
> + X86CPU *cpu;
> int i;
>
> for(i = 0; i < smp_cpus; i++) {
If we do changes here, please add the missing space. :)
> - pc_new_cpu(cpu_model);
> + cpu = cpu_x86_init(cpu_model);
> + if (cpu == NULL) {
> + exit(1);
> + }
> + qemu_register_reset(pc_cpu_reset, cpu);
> }
> }
>
> diff --git a/target-i386/cpu.c b/target-i386/cpu.c
> index e655129..99ef891 100644
> --- a/target-i386/cpu.c
> +++ b/target-i386/cpu.c
> @@ -1749,24 +1753,89 @@ 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);
> + qdev_prop_set_ptr(env->apic_state, "cpu_env", env);
I'd like to avoid re-adding this set_ptr(). We can cherry-pick my
link<X86CPU> property from QOM CPUState part 4 series.
> + }
> +#endif
> +}
> +
> +#ifndef CONFIG_USER_ONLY
> +static CPUDebugExcpHandler *prev_debug_excp_handler;
> +
> +static void breakpoint_handler(CPUX86State *env)
> +{
> + CPUBreakpoint *bp;
> +
> + if (env->watchpoint_hit) {
> + if (env->watchpoint_hit->flags & BP_CPU) {
> + env->watchpoint_hit = NULL;
> + if (check_hw_breakpoints(env, 0)) {
> + raise_exception_env(EXCP01_DB, env);
> + } else {
> + cpu_resume_from_signal(env, NULL);
> + }
> + }
> + } else {
> + QTAILQ_FOREACH(bp, &env->breakpoints, entry)
> + if (bp->pc == env->eip) {
> + if (bp->flags & BP_CPU) {
> + check_hw_breakpoints(env, 1);
> + raise_exception_env(EXCP01_DB, env);
> + }
> + break;
> + }
> + }
> + if (prev_debug_excp_handler) {
> + prev_debug_excp_handler(env);
> }
> }
> +#endif
>
> void x86_cpu_realize(Object *obj, Error **errp)
> {
> X86CPU *cpu = X86_CPU(obj);
> +#ifndef CONFIG_USER_ONLY
> + CPUX86State *env = &cpu->env;
> +
> + if (env->apic_state) {
> + if (qdev_init(env->apic_state) < 0) {
> + error_set(errp, QERR_DEVICE_INIT_FAILED,
> + object_get_typename(OBJECT(env->apic_state)));
> + return;
> + }
> + }
> +#endif
>
> mce_init(cpu);
> - qemu_init_vcpu(&cpu->env);
> + qemu_init_vcpu(env);
This only works because currently qemu_init_vcpu() is a no-op macro that
doesn't use the parameter. Please don't change it back, I guess it's a
mismerge.
We can avoid the env variable if I finish merging Paolo's series - by
realizing the CPU the APIC as its child would get realized, too. Is the
ordering before mce_init() mandatory here or is it just to reduce the
#ifndef'ery?
> + cpu_reset(CPU(cpu));
> }
>
> static void x86_cpu_initfn(Object *obj)
> {
> X86CPU *cpu = X86_CPU(obj);
> CPUX86State *env = &cpu->env;
> + static int inited;
>
> cpu_exec_init(env);
>
> + env->cpuid_apic_id = env->cpu_index;
> +
> object_property_add(obj, "family", "int",
> x86_cpuid_version_get_family,
> x86_cpuid_version_set_family, NULL, NULL, NULL);
> @@ -1795,7 +1864,15 @@ static void x86_cpu_initfn(Object *obj)
> object_property_add_str(obj, "cpu-model",
> x86_get_cpu_model, x86_set_cpu_model, NULL);
>
> - env->cpuid_apic_id = env->cpu_index;
> + /* init various static tables used in TCG mode */
> + if (tcg_enabled() && !inited) {
> + inited = 1;
> + optimize_flags_init();
> +#ifndef CONFIG_USER_ONLY
> + prev_debug_excp_handler =
> + cpu_set_debug_excp_handler(breakpoint_handler);
> +#endif
> + }
Did you forget to put that into its own patch or did that not work?
My idea was to have it first in the series so that other changes here
and elsewhere can be rebased onto it.
Also I wonder whether it would better be placed into the class_init? I'd
tend towards initfn because that will not be invoked during type
enumeration.
> }
>
> static void x86_cpu_common_class_init(ObjectClass *oc, void *data)
> diff --git a/target-i386/helper.c b/target-i386/helper.c
> index fbaeeea..38ac25d 100644
> --- a/target-i386/helper.c
> +++ b/target-i386/helper.c
> @@ -941,34 +941,6 @@ int check_hw_breakpoints(CPUX86State *env, int
> force_dr6_update)
> return hit_enabled;
> }
>
> -static CPUDebugExcpHandler *prev_debug_excp_handler;
> -
> -static void breakpoint_handler(CPUX86State *env)
> -{
> - CPUBreakpoint *bp;
> -
> - if (env->watchpoint_hit) {
> - if (env->watchpoint_hit->flags & BP_CPU) {
> - env->watchpoint_hit = NULL;
> - if (check_hw_breakpoints(env, 0))
> - raise_exception_env(EXCP01_DB, env);
> - else
> - cpu_resume_from_signal(env, NULL);
> - }
> - } else {
> - QTAILQ_FOREACH(bp, &env->breakpoints, entry)
> - if (bp->pc == env->eip) {
> - if (bp->flags & BP_CPU) {
> - check_hw_breakpoints(env, 1);
> - raise_exception_env(EXCP01_DB, env);
> - }
> - break;
> - }
> - }
> - if (prev_debug_excp_handler)
> - prev_debug_excp_handler(env);
> -}
> -
I wonder if that could rather stay here as non-static?
> typedef struct MCEInjectionParams {
> Monitor *mon;
> CPUX86State *env;
> @@ -1155,20 +1127,9 @@ X86CPU *cpu_x86_init(const char *cpu_model)
> {
> X86CPU *cpu;
> Error *errp = NULL;
> - static int inited;
>
> cpu = X86_CPU(object_new(TYPE_X86_CPU));
>
> - /* init various static tables used in TCG mode */
> - if (tcg_enabled() && !inited) {
> - inited = 1;
> - optimize_flags_init();
> -#ifndef CONFIG_USER_ONLY
> - prev_debug_excp_handler =
> - cpu_set_debug_excp_handler(breakpoint_handler);
> -#endif
> - }
> -
> if (cpu_model) {
> object_property_set_str(OBJECT(cpu), cpu_model, "cpu-model", &errp);
> } else {
/-F
--
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg
- [Qemu-devel] [PATCH qom-next v2 0/6] target-i386: re-factor CPU creation/initialization to QOM, Igor Mammedov, 2012/05/23
- [Qemu-devel] [PATCH qom-next 2/6] target-i386: move cpu halted decision into x86_cpu_reset, Igor Mammedov, 2012/05/23
- [Qemu-devel] [PATCH qom-next 1/6] pc: Enable MSI support at APIC level, Igor Mammedov, 2012/05/23
- [Qemu-devel] [PATCH qom-next 5/6] target-i386: make initialize CPU in QOM way, Igor Mammedov, 2012/05/23
- Re: [Qemu-devel] [PATCH qom-next 5/6] target-i386: make initialize CPU in QOM way,
Andreas Färber <=
- [Qemu-devel] [PATCH qom-next 3/6] target-i386: add cpu-model property to x86_cpu, Igor Mammedov, 2012/05/23
- [Qemu-devel] [PATCH qom-next 4/6] pc: move apic_mapped initialization into common apic init code, Igor Mammedov, 2012/05/23
- Re: [Qemu-devel] [PATCH qom-next 4/6] pc: move apic_mapped initialization into common apic init code, Peter Maydell, 2012/05/23
- Re: [Qemu-devel] [PATCH qom-next 4/6] pc: move apic_mapped initialization into common apic init code, Jan Kiszka, 2012/05/23
- Re: [Qemu-devel] [PATCH qom-next 4/6] pc: move apic_mapped initialization into common apic init code, Igor Mammedov, 2012/05/23
- Re: [Qemu-devel] [PATCH qom-next 4/6] pc: move apic_mapped initialization into common apic init code, Peter Maydell, 2012/05/23
- Re: [Qemu-devel] [PATCH qom-next 4/6] pc: move apic_mapped initialization into common apic init code, Igor Mammedov, 2012/05/24
- Re: [Qemu-devel] [PATCH qom-next 4/6] pc: move apic_mapped initialization into common apic init code, Peter Maydell, 2012/05/24
- Re: [Qemu-devel] [PATCH qom-next 4/6] pc: move apic_mapped initialization into common apic init code, Jan Kiszka, 2012/05/24