[Top][All Lists]

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Qemu-devel] [PATCH 3/5] target-i386: call x86_cpu_realize() after A

From: Igor Mammedov
Subject: Re: [Qemu-devel] [PATCH 3/5] target-i386: call x86_cpu_realize() after APIC is initialized.
Date: Wed, 11 Jul 2012 14:27:04 +0200
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:13.0) Gecko/20120615 Thunderbird/13.0.1

On 07/11/2012 09:35 AM, liu ping fan wrote:
On Tue, Jul 10, 2012 at 9:35 PM, Igor Mammedov <address@hidden> wrote:
On 07/09/2012 02:57 PM, Andreas Färber wrote:

Am 09.07.2012 12:59, schrieb igor:

On 06/20/2012 03:35 PM, Andreas Färber wrote:

Am 20.06.2012 14:59, schrieb Igor Mammedov:

It's not correct to make CPU runnable (i.e. calling x86_cpu_realize())
when not all properties are set (APIC in this case).

Fix it by calling x86_cpu_realize() at board level after APIC is
initialized, right before cpu_reset().

Signed-off-by: Igor Mammedov <address@hidden>
    hw/pc.c              |    1 +
    target-i386/helper.c |    2 --
    2 files changed, 1 insertion(+), 2 deletions(-)

diff --git a/hw/pc.c b/hw/pc.c
index 8368701..8a662cf 100644
--- a/hw/pc.c
+++ b/hw/pc.c
@@ -948,6 +948,7 @@ static X86CPU *pc_new_cpu(const char *cpu_model)
            env->apic_state = apic_init(env, env->cpuid_apic_id);
        qemu_register_reset(pc_cpu_reset, cpu);
+    x86_cpu_realize(OBJECT(cpu), NULL);
        return cpu;
diff --git a/target-i386/helper.c b/target-i386/helper.c
index c52ec13..b38ea7f 100644
--- a/target-i386/helper.c
+++ b/target-i386/helper.c
@@ -1161,8 +1161,6 @@ X86CPU *cpu_x86_init(const char *cpu_model)
            return NULL;

-    x86_cpu_realize(OBJECT(cpu), NULL);
        return cpu;

This will require changes in linux-user and possibly bsd-user. Having a
cpu_realize() would probably help with avoiding #ifdef'ery.
Unfortunately deriving CPUState from DeviceState proves a bit difficult
in the meantime (it worked at one point, now there's lots of circular
header dependencies), and realize support for Object got stopped.

As alternative to keep, I could leave x86_cpu_realize() in
cpu_x86_init() and keep pc_cpu_reset() in pc_new_cpu(). That will result
in calling cpu_reset() 3 instead of 2 times.
Later when apic_init is moved inside cpu.c, a pc_cpu_reset() in
pc_new_cpu() would be unnecessary and could be cleaned up then.

Let me explain in more detail what I was thinking about: cpu_init() and
cpu_x86_init() today return an initialized/realized object. I don't want
bugs to creep into the user emulators because someone is not aware that
x86 is semantically differing from all other targets.

What I did for a qemu-rl78 machine is to inline cpu_rl78_init() so that
I could put code in between, i.e., for x86: object_new(), APIC/BSP
stuff, x86_cpu_realize(). That way any addition to the realize function
will still affect the user emulators.
The downside is that when we add x86 CPU subclasses we'd have to
remember to update two places. The solution to that would be to split
out a x86_cpu_new() function used from cpu_x86_init() and wherever you
need it for the PC machine. Then the code is still maintainable in one
central place and you get to do your APIC cleanups, and we don't depend
on a central realize implementation or device parent, what do you think?

If you mean x86_cpu_new() == pc_new_cpu() that calls cpu_x86_init(),
then I'd like get rid of pc_new_cpu() completely, eventually replacing it by
cpu_x86_init() in hw/pc.c:pc_cpus_init(), something like this:

-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;
@@ -950,7 +932,7 @@ void pc_cpus_init(const char *cpu_model)

      for(i = 0; i < smp_cpus; i++) {
-        pc_new_cpu(cpu_model);
+        cpu_x86_init(cpu_model);

goal I'm aiming at is to have a working cpu object that could be created
using qdev_device_add without any adhoc calls. So in the end cpu_x86_init()
should become object_new(x86_cpu), [set props], realize() and nothing else.

Could we think apic's "creation + realize" as part of
x86_cpu_realize(), but not [set props]?
For the concept of building sub log unit inside chip.

Yes, sure.
Please look at https://github.com/imammedo/qemu/tree/x86_qom_apic
it lacks apic_reset() from cpu_reset() but it is easy to add.

And maybe in some far future removing cpu_init -> cpu_x86_init() completely.
That would give us a single implementation of CPU one place (cpu.c)


reply via email to

[Prev in Thread] Current Thread [Next in Thread]