[Qemu-devel] [PATCH 3/4] move halted state setting to inside of cpu_x86_init

[Qemu-devel] [PATCH 3/4] move halted state setting to inside of cpu_x86_init

[Glauber Costa]

cpus other than the first one starting at the halted state
is a safe assumption to anyone. Move it to where it belongs.

Signed-off-by: Glauber Costa <glommer@redhat.com>
---
 hw/pc.c              |    2 --
 target-i386/helper.c |    3 +++
 2 files changed, 3 insertions(+), 2 deletions(-)

diff --git a/hw/pc.c b/hw/pc.c
index b726c17..06d1fca 100644
--- a/hw/pc.c
+++ b/hw/pc.c
@@ -849,8 +849,6 @@ static void pc_init1(ram_addr_t ram_size, int vga_ram_size,
             fprintf(stderr, "Unable to find x86 CPU definition\n");
             exit(1);
         }
-        if (i != 0)
-            env->halted = 1;
         if (pci_enabled) {
             apic_init(env);
         }
diff --git a/target-i386/helper.c b/target-i386/helper.c
index 2c11cd3..a0d0273 100644
--- a/target-i386/helper.c
+++ b/target-i386/helper.c
@@ -1694,6 +1694,9 @@ CPUX86State *cpu_x86_init(const char *cpu_model)
         cpu_x86_close(env);
         return NULL;
     }
+
+    /* cpu 0 can run, others start at halted state */
+    env->halted = !!env->cpu_index;
     cpu_reset(env);
     qemu_register_reset(main_cpu_reset, env);
 
-- 
1.5.6.6

Re: [Qemu-devel] [PATCH 3/4] move halted state setting to inside of cpu_x86_init

[Blue Swirl]

On 5/6/09, Glauber Costa <glommer@redhat.com> wrote:
> cpus other than the first one starting at the halted state
>  is a safe assumption to anyone. Move it to where it belongs.

The code should be conditional to !CONFIG_USER_ONLY.

The same happens with Sparc, so I'd move this to exec.c:cpu_exec_init().

Re: [Qemu-devel] [PATCH 3/4] move halted state setting to inside of cpu_x86_init

[Glauber Costa]

On Wed, May 06, 2009 at 10:31:15PM +0300, Blue Swirl wrote:
> On 5/6/09, Glauber Costa <glommer@redhat.com> wrote:
> > cpus other than the first one starting at the halted state
> >  is a safe assumption to anyone. Move it to where it belongs.
> 
> The code should be conditional to !CONFIG_USER_ONLY.
Fine.
> 
> The same happens with Sparc, so I'd move this to exec.c:cpu_exec_init().
It would be indeed, better.

Anyone from other arches opposes to this?

[Qemu-devel] [PATCH 0/4] Simplify cpu_init

[Glauber Costa]

This is the same series I sent yesterday, but with comments addressed.

[Qemu-devel] [PATCH 1/4] move registering of cpu_reset to inside cpu_init

[Glauber Costa]

This is not pc specific by any means. So we can be
moved to inside cpu_x86_init().

This is part of an attempt to only initialize kvm state
after everything is already properly initialized. If we don't
do that, we can race against, for example, APIC state if kvm vcpus
are ran in threads (happens in qemu-kvm.git, soon to happen here too)

Signed-off-by: Glauber Costa <glommer@redhat.com>
---
 hw/pc.c              |    7 -------
 target-i386/helper.c |    8 ++++++++
 2 files changed, 8 insertions(+), 7 deletions(-)

diff --git a/hw/pc.c b/hw/pc.c
index 61f6e7b..351de83 100644
--- a/hw/pc.c
+++ b/hw/pc.c
@@ -725,12 +725,6 @@ static void load_linux(target_phys_addr_t option_rom,
     generate_bootsect(option_rom, gpr, seg, 0);
 }
 
-static void main_cpu_reset(void *opaque)
-{
-    CPUState *env = opaque;
-    cpu_reset(env);
-}
-
 static const int ide_iobase[2] = { 0x1f0, 0x170 };
 static const int ide_iobase2[2] = { 0x3f6, 0x376 };
 static const int ide_irq[2] = { 14, 15 };
@@ -861,7 +855,6 @@ static void pc_init1(ram_addr_t ram_size, int vga_ram_size,
             /* XXX: enable it in all cases */
             env->cpuid_features |= CPUID_APIC;
         }
-        qemu_register_reset(main_cpu_reset, env);
         if (pci_enabled) {
             apic_init(env);
         }
diff --git a/target-i386/helper.c b/target-i386/helper.c
index a070e08..2210412 100644
--- a/target-i386/helper.c
+++ b/target-i386/helper.c
@@ -29,6 +29,7 @@
 #include "exec-all.h"
 #include "qemu-common.h"
 #include "kvm.h"
+#include "hw/hw.h"
 
 //#define DEBUG_MMU
 
@@ -507,6 +508,11 @@ void cpu_reset(CPUX86State *env)
     cpu_watchpoint_remove_all(env, BP_CPU);
 }
 
+static void main_cpu_reset(void *_env)
+{
+    cpu_reset((CPUState *)_env);
+}
+
 void cpu_x86_close(CPUX86State *env)
 {
     qemu_free(env);
@@ -1689,6 +1695,8 @@ CPUX86State *cpu_x86_init(const char *cpu_model)
         return NULL;
     }
     cpu_reset(env);
+    qemu_register_reset(main_cpu_reset, env);
+
 #ifdef CONFIG_KQEMU
     kqemu_init(env);
 #endif
-- 
1.5.6.6

[Qemu-devel] [PATCH 2/4] move CPUID_APIC flag to where it belongs

[Glauber Costa]

We can safely do that inconditionally, so move to processor defined
flags like any other flag. ISA machines will still see a LAPIC, but
it should behave fine as a normal PIC.

Signed-off-by: Glauber Costa <glommer@redhat.com>
---
 hw/pc.c              |    4 ----
 target-i386/helper.c |    2 +-
 2 files changed, 1 insertions(+), 5 deletions(-)

diff --git a/hw/pc.c b/hw/pc.c
index 351de83..b726c17 100644
--- a/hw/pc.c
+++ b/hw/pc.c
@@ -851,10 +851,6 @@ static void pc_init1(ram_addr_t ram_size, int vga_ram_size,
         }
         if (i != 0)
             env->halted = 1;
-        if (smp_cpus > 1) {
-            /* XXX: enable it in all cases */
-            env->cpuid_features |= CPUID_APIC;
-        }
         if (pci_enabled) {
             apic_init(env);
         }
diff --git a/target-i386/helper.c b/target-i386/helper.c
index 2210412..2c11cd3 100644
--- a/target-i386/helper.c
+++ b/target-i386/helper.c
@@ -102,7 +102,7 @@ typedef struct x86_def_t {
     char model_id[48];
 } x86_def_t;
 
-#define I486_FEATURES (CPUID_FP87 | CPUID_VME | CPUID_PSE)
+#define I486_FEATURES (CPUID_FP87 | CPUID_VME | CPUID_PSE | CPUID_APIC)
 #define PENTIUM_FEATURES (I486_FEATURES | CPUID_DE | CPUID_TSC | \
           CPUID_MSR | CPUID_MCE | CPUID_CX8 | CPUID_MMX)
 #define PENTIUM2_FEATURES (PENTIUM_FEATURES | CPUID_PAE | CPUID_SEP | \
-- 
1.5.6.6

[Qemu-devel] [PATCH 3/4] move halted state setting to inside of cpu_x86_init

[Glauber Costa]

cpus other than the first one starting at the halted state
is a safe assumption to anyone. Move it to exec.c, in common
cpu initialization code. Also, we need to keep this state when
we reset the cpu.

Signed-off-by: Glauber Costa <glommer@redhat.com>
---
 exec.c               |    4 ++++
 hw/pc.c              |    2 --
 hw/sun4m.c           |    2 --
 target-i386/helper.c |    4 ++++
 4 files changed, 8 insertions(+), 4 deletions(-)

diff --git a/exec.c b/exec.c
index c5c9280..84a8e48 100644
--- a/exec.c
+++ b/exec.c
@@ -567,6 +567,10 @@ void cpu_exec_init(CPUState *env)
     register_savevm("cpu", cpu_index, CPU_SAVE_VERSION,
                     cpu_save, cpu_load, env);
 #endif
+#if !defined(CONFIG_USER_ONLY)
+    /* cpu 0 can run, others start at halted state */
+    env->halted = !!cpu_index;
+#endif
 }
 
 static inline void invalidate_page_bitmap(PageDesc *p)
diff --git a/hw/pc.c b/hw/pc.c
index b726c17..06d1fca 100644
--- a/hw/pc.c
+++ b/hw/pc.c
@@ -849,8 +849,6 @@ static void pc_init1(ram_addr_t ram_size, int vga_ram_size,
             fprintf(stderr, "Unable to find x86 CPU definition\n");
             exit(1);
         }
-        if (i != 0)
-            env->halted = 1;
         if (pci_enabled) {
             apic_init(env);
         }
diff --git a/hw/sun4m.c b/hw/sun4m.c
index 1f1efd0..856b636 100644
--- a/hw/sun4m.c
+++ b/hw/sun4m.c
@@ -403,7 +403,6 @@ static void sun4m_hw_init(const struct sun4m_hwdef *hwdef, ram_addr_t RAM_size,
             qemu_register_reset(main_cpu_reset, env);
         } else {
             qemu_register_reset(secondary_cpu_reset, env);
-            env->halted = 1;
         }
         cpu_irqs[i] = qemu_allocate_irqs(cpu_set_irq, envs[i], MAX_PILS);
         env->prom_addr = hwdef->slavio_base;
@@ -1193,7 +1192,6 @@ static void sun4d_hw_init(const struct sun4d_hwdef *hwdef, ram_addr_t RAM_size,
             qemu_register_reset(main_cpu_reset, env);
         } else {
             qemu_register_reset(secondary_cpu_reset, env);
-            env->halted = 1;
         }
         cpu_irqs[i] = qemu_allocate_irqs(cpu_set_irq, envs[i], MAX_PILS);
         env->prom_addr = hwdef->slavio_base;
diff --git a/target-i386/helper.c b/target-i386/helper.c
index 2c11cd3..3131757 100644
--- a/target-i386/helper.c
+++ b/target-i386/helper.c
@@ -506,6 +506,10 @@ void cpu_reset(CPUX86State *env)
     env->dr[7] = DR7_FIXED_1;
     cpu_breakpoint_remove_all(env, BP_CPU);
     cpu_watchpoint_remove_all(env, BP_CPU);
+#if !defined(CONFIG_USER_ONLY)
+    /* cpu 0 can run, others start at halted state */
+    env->halted = !!env->cpu_index;
+#endif
 }
 
 static void main_cpu_reset(void *_env)
-- 
1.5.6.6

[Qemu-devel] [PATCH 4/4] move apic functions to a separate apic.h header

[Glauber Costa]

Later on, we'll want to call an apic function from helper.c.
The inclusion of pc.h, besides totally ugly, leads to a lot of
clashes.

Signed-off-by: Glauber Costa <glommer@redhat.com>
---
 hw/apic.c        |    1 +
 hw/apic.h        |   19 +++++++++++++++++++
 hw/ioapic.c      |    1 +
 hw/mc146818rtc.c |    1 +
 hw/pc.c          |    1 +
 hw/pc.h          |   15 ---------------
 6 files changed, 23 insertions(+), 15 deletions(-)
 create mode 100644 hw/apic.h

diff --git a/hw/apic.c b/hw/apic.c
index d63d74b..72dbe88 100644
--- a/hw/apic.c
+++ b/hw/apic.c
@@ -19,6 +19,7 @@
  */
 #include "hw.h"
 #include "pc.h"
+#include "apic.h"
 #include "qemu-timer.h"
 #include "host-utils.h"
 
diff --git a/hw/apic.h b/hw/apic.h
new file mode 100644
index 0000000..2437e9f
--- /dev/null
+++ b/hw/apic.h
@@ -0,0 +1,19 @@
+#ifndef _APIC_H_
+#define _APIC_H_
+
+/* APIC */
+typedef struct IOAPICState IOAPICState;
+void apic_deliver_irq(uint8_t dest, uint8_t dest_mode,
+                             uint8_t delivery_mode,
+                             uint8_t vector_num, uint8_t polarity,
+                             uint8_t trigger_mode);
+int apic_init(CPUState *env);
+int apic_accept_pic_intr(CPUState *env);
+void apic_deliver_pic_intr(CPUState *env, int level);
+int apic_get_interrupt(CPUState *env);
+IOAPICState *ioapic_init(void);
+void ioapic_set_irq(void *opaque, int vector, int level);
+void apic_reset_irq_delivered(void);
+int apic_get_irq_delivered(void);
+
+#endif
diff --git a/hw/ioapic.c b/hw/ioapic.c
index 317c2c2..064f9ce 100644
--- a/hw/ioapic.c
+++ b/hw/ioapic.c
@@ -22,6 +22,7 @@
  */
 
 #include "hw.h"
+#include "apic.h"
 #include "pc.h"
 #include "qemu-timer.h"
 #include "host-utils.h"
diff --git a/hw/mc146818rtc.c b/hw/mc146818rtc.c
index 888b85a..b71624f 100644
--- a/hw/mc146818rtc.c
+++ b/hw/mc146818rtc.c
@@ -25,6 +25,7 @@
 #include "qemu-timer.h"
 #include "sysemu.h"
 #include "pc.h"
+#include "apic.h"
 #include "isa.h"
 #include "hpet_emul.h"
 
diff --git a/hw/pc.c b/hw/pc.c
index 06d1fca..2035705 100644
--- a/hw/pc.c
+++ b/hw/pc.c
@@ -23,6 +23,7 @@
  */
 #include "hw.h"
 #include "pc.h"
+#include "apic.h"
 #include "fdc.h"
 #include "pci.h"
 #include "block.h"
diff --git a/hw/pc.h b/hw/pc.h
index 50e6c39..417ff65 100644
--- a/hw/pc.h
+++ b/hw/pc.h
@@ -40,21 +40,6 @@ uint32_t pic_intack_read(PicState2 *s);
 void pic_info(Monitor *mon);
 void irq_info(Monitor *mon);
 
-/* APIC */
-typedef struct IOAPICState IOAPICState;
-void apic_deliver_irq(uint8_t dest, uint8_t dest_mode,
-                             uint8_t delivery_mode,
-                             uint8_t vector_num, uint8_t polarity,
-                             uint8_t trigger_mode);
-int apic_init(CPUState *env);
-int apic_accept_pic_intr(CPUState *env);
-void apic_deliver_pic_intr(CPUState *env, int level);
-int apic_get_interrupt(CPUState *env);
-IOAPICState *ioapic_init(void);
-void ioapic_set_irq(void *opaque, int vector, int level);
-void apic_reset_irq_delivered(void);
-int apic_get_irq_delivered(void);
-
 /* i8254.c */
 
 #define PIT_FREQ 1193182
-- 
1.5.6.6

Re: [Qemu-devel] [PATCH 4/4] move apic functions to a separate apic.h header

[malc]

On Thu, 7 May 2009, Glauber Costa wrote:

> Later on, we'll want to call an apic function from helper.c.
> The inclusion of pc.h, besides totally ugly, leads to a lot of
> clashes.

[..snip..]

> +#ifndef _APIC_H_
> +#define _APIC_H_

Looks like you forgot to fix that.

[..snip..]

-- 
mailto:av1474@comtv.ru

Re: [Qemu-devel] [PATCH 4/4] move apic functions to a separate apic.h header

[Glauber Costa]

On Thu, May 07, 2009 at 11:12:17PM +0400, malc wrote:
> On Thu, 7 May 2009, Glauber Costa wrote:
> 
> > Later on, we'll want to call an apic function from helper.c.
> > The inclusion of pc.h, besides totally ugly, leads to a lot of
> > clashes.
> 
> [..snip..]
> 
> > +#ifndef _APIC_H_
> > +#define _APIC_H_
> 
> Looks like you forgot to fix that.
ughhh

you're right.

Anthony, mind committing the other patches? (if they are okay?)
I can then send just this one, to avoid confusion