[Qemu-devel] [PATCH 0/4] Simplify cpu initialization

[Qemu-devel] [PATCH 0/4] Simplify cpu initialization

[Glauber Costa]

Currently, there are pieces of cpu initialization being done in two
different places. The big part of it, is done inside cpu_x86_init,
while minor others are done in pc.c, after cpu_x86_init returns.

For things like halting cpus != 0, registering reset handler, etc,
there is no reason whatsoever for not doing it in cpu  initialization.

The apic initialization should be moved there too, and theorectically,
it should not depend on pci machine. However, it currently breaks
our isa machine, and to avoid a hack, it is left where it is.

[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.

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 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

[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

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

[Jan Kiszka]

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.
> 
> 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"

Does it still need pc.h then?

>  #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_H", otherwise fine.

> +
> +/* 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

Jan

-- 
Siemens AG, Corporate Technology, CT SE 2
Corporate Competence Center Embedded Linux

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

[Glauber Costa]

On Wed, May 06, 2009 at 05:52:04PM +0200, Jan Kiszka wrote:
> 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.
> > 
> > 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"
> 
> Does it still need pc.h then?
yes. For things like RTCState.

> 
> >  #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_H", otherwise fine.
ok.

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

[Jan Kiszka]

Glauber Costa wrote:
> 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);
>  

That looks good to me.

Jan


-- 
Siemens AG, Corporate Technology, CT SE 2
Corporate Competence Center Embedded Linux

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] Re: [PATCH 2/4] move CPUID_APIC flag to where it belongs

[Jan Kiszka]

Glauber Costa wrote:
> We can safely do that inconditionally, so move to processor defined
> flags like any other flag.
> 
> 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)

Without doing my homework:
What impact will it have on an emulated CPUs without [L]APIC? I'm
thinking of -M isapc e.g.

Jan

-- 
Siemens AG, Corporate Technology, CT SE 2
Corporate Competence Center Embedded Linux

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

[Glauber Costa]

On Wed, May 06, 2009 at 05:40:40PM +0200, Jan Kiszka wrote:
> Glauber Costa wrote:
> > We can safely do that inconditionally, so move to processor defined
> > flags like any other flag.
> > 
> > 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)
> 
> Without doing my homework:
> What impact will it have on an emulated CPUs without [L]APIC? I'm
> thinking of -M isapc e.g.
I tried, and it still boots fine.
The flag just indicates it supports an apic, not that it has one.

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

[Stanislav]

If OS boot fine its more luck. The flag indicates presence of local apic,
not "if it is supported".
For example when lapic is globally disabled (i.e. not present) CPUID flag is
cleared off as well.
But CPU is still supports lapic, after reset it will be back.

Stanislav


> Without doing my homework:
> What impact will it have on an emulated CPUs without [L]APIC? I'm
> thinking of -M isapc e.g.


>I tried, and it still boots fine.
>The flag just indicates it supports an apic, not that it has one.

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

[Glauber Costa]

On Wed, May 06, 2009 at 08:29:23PM +0300, Stanislav wrote:
> If OS boot fine its more luck. The flag indicates presence of local apic,
> not "if it is supported".
> For example when lapic is globally disabled (i.e. not present) CPUID flag is
> cleared off as well.
> But CPU is still supports lapic, after reset it will be back.
What if we take out a cpu that supports lapic, and is connected to an apic system,
and connects it to a board that has no apic?

Also, note that in the old code, it was set regardless of the existance of an APIC
anyway. The only dependancy was on smp_cpus > 1.

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

[Stanislav]

Local apic and i/o apic are not really related.
Local apic is actually a cpu feature, not optional external peripheral.
I/O apic is external hardware on board and if you don't have it - you can't
program your interrupts that much.
I am not telling if a patch is wrong/better than it was before - just want
to point out how it really works.

Stanislav


-----Original Message-----
From: Glauber Costa [mailto:glommer@redhat.com] 
Sent: Wednesday, May 06, 2009 8:42 PM
To: Stanislav
Cc: 'Jan Kiszka'; aliguori@us.ibm.com; qemu-devel@nongnu.org
Subject: Re: [Qemu-devel] Re: [PATCH 2/4] move CPUID_APIC flag to where it
belongs

On Wed, May 06, 2009 at 08:29:23PM +0300, Stanislav wrote:
> If OS boot fine its more luck. The flag indicates presence of local apic,
> not "if it is supported".
> For example when lapic is globally disabled (i.e. not present) CPUID flag
is
> cleared off as well.
> But CPU is still supports lapic, after reset it will be back.
What if we take out a cpu that supports lapic, and is connected to an apic
system,
and connects it to a board that has no apic?

Also, note that in the old code, it was set regardless of the existance of
an APIC
anyway. The only dependancy was on smp_cpus > 1.

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

[Anthony Liguori]

Stanislav wrote:
> If OS boot fine its more luck. The flag indicates presence of local apic,
> not "if it is supported".
>   

But having a local apic present in an isapc should not cause any 
problems as it's backwards compatible with the pic.

But yeah, there really isn't a "is supported" flag.  It's either there 
or it's not.

Regards,

Anthony Liguori

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

[Jan Kiszka]

Glauber Costa wrote:
> 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

After digging through the dependencies of apic and cpu again, this looks
safe (it reorders reset execution, but that should have no side effect).

Jan

-- 
Siemens AG, Corporate Technology, CT SE 2
Corporate Competence Center Embedded Linux

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

[Blue Swirl]

On 5/6/09, Glauber Costa <glommer@redhat.com> wrote:
> 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)

I'd move this too to exec.c. Sparc registers two reset functions, the
first one for the main cpu disables halted state, the second one for
non-boot cpus halts them on reset. I wonder why i386 does not do the
same.

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

[Glauber Costa]

On Wed, May 06, 2009 at 10:36:16PM +0300, Blue Swirl wrote:
> On 5/6/09, Glauber Costa <glommer@redhat.com> wrote:
> > 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)
> 
> I'd move this too to exec.c. Sparc registers two reset functions, the
> first one for the main cpu disables halted state, the second one for
> non-boot cpus halts them on reset. I wonder why i386 does not do the
> same.

Probably because we were halting the cpu separatedly.
Btw, after looking again, cpu_reset looks like an even better place
to hold the halted change.

[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