[Qemu-devel] [PATCH 1/2] Apic creation should not depend on pci

[Qemu-devel] [PATCH 1/2] Apic creation should not depend on pci

[Gleb Natapov]

It should depend on whether cpu has APIC.

Signed-off-by: Gleb Natapov <gleb@redhat.com>
---
 hw/pc.c |    6 +-----
 1 files changed, 1 insertions(+), 5 deletions(-)

diff --git a/hw/pc.c b/hw/pc.c
index 0934778..d5b4112 100644
--- a/hw/pc.c
+++ b/hw/pc.c
@@ -878,12 +878,8 @@ static void pc_init1(ram_addr_t ram_size,
         }
         if (i != 0)
             env->halted = 1;
-        if (smp_cpus > 1) {
-            /* XXX: enable it in all cases */
-            env->cpuid_features |= CPUID_APIC;
-        }
         qemu_register_reset(main_cpu_reset, 0, env);
-        if (pci_enabled) {
+        if ((env->cpuid_features & CPUID_APIC) || smp_cpus > 1) {
             apic_init(env);
         }
     }
-- 
1.6.2.1

[Qemu-devel] [PATCH 2/2] Don't register cpu reset handler for cpu with APIC.

[Gleb Natapov]

APIC reset handler already resets cpu. Also register cpu_reset handler
directly to make it impossible to add additional code to main_cpu_reset()
by mistake.

Signed-off-by: Gleb Natapov <gleb@redhat.com>
---
 hw/pc.c |   10 +++-------
 1 files changed, 3 insertions(+), 7 deletions(-)

diff --git a/hw/pc.c b/hw/pc.c
index d5b4112..5e7b115 100644
--- a/hw/pc.c
+++ b/hw/pc.c
@@ -751,12 +751,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 };
@@ -878,9 +872,11 @@ static void pc_init1(ram_addr_t ram_size,
         }
         if (i != 0)
             env->halted = 1;
-        qemu_register_reset(main_cpu_reset, 0, env);
         if ((env->cpuid_features & CPUID_APIC) || smp_cpus > 1) {
             apic_init(env);
+        } else {
+	    /* APIC reset callback resets cpu */
+            qemu_register_reset((QEMUResetHandler*)cpu_reset, 0, env);
         }
     }
 
-- 
1.6.2.1

[Qemu-devel] Re: [PATCH 2/2] Don't register cpu reset handler for cpu with APIC.

[Jan Kiszka]

Gleb Natapov wrote:
> APIC reset handler already resets cpu. Also register cpu_reset handler
> directly to make it impossible to add additional code to main_cpu_reset()
> by mistake.
> 
> Signed-off-by: Gleb Natapov <gleb@redhat.com>
> ---
>  hw/pc.c |   10 +++-------
>  1 files changed, 3 insertions(+), 7 deletions(-)
> 
> diff --git a/hw/pc.c b/hw/pc.c
> index d5b4112..5e7b115 100644
> --- a/hw/pc.c
> +++ b/hw/pc.c
> @@ -751,12 +751,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 };
> @@ -878,9 +872,11 @@ static void pc_init1(ram_addr_t ram_size,
>          }
>          if (i != 0)
>              env->halted = 1;
> -        qemu_register_reset(main_cpu_reset, 0, env);
>          if ((env->cpuid_features & CPUID_APIC) || smp_cpus > 1) {
>              apic_init(env);
> +        } else {
> +	    /* APIC reset callback resets cpu */

That comment should rather go into the previous block (and please remove
the tab).

> +            qemu_register_reset((QEMUResetHandler*)cpu_reset, 0, env);
>          }
>      }
>  

Looks fine to me otherwise. Will give these patches a try later today.

Jan

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

[Qemu-devel] Re: [PATCH 2/2] Don't register cpu reset handler for cpu with APIC.

[Gleb Natapov]

On Tue, Jun 09, 2009 at 12:21:09PM +0200, Jan Kiszka wrote:
> Gleb Natapov wrote:
> > APIC reset handler already resets cpu. Also register cpu_reset handler
> > directly to make it impossible to add additional code to main_cpu_reset()
> > by mistake.
> > 
> > Signed-off-by: Gleb Natapov <gleb@redhat.com>
> > ---
> >  hw/pc.c |   10 +++-------
> >  1 files changed, 3 insertions(+), 7 deletions(-)
> > 
> > diff --git a/hw/pc.c b/hw/pc.c
> > index d5b4112..5e7b115 100644
> > --- a/hw/pc.c
> > +++ b/hw/pc.c
> > @@ -751,12 +751,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 };
> > @@ -878,9 +872,11 @@ static void pc_init1(ram_addr_t ram_size,
> >          }
> >          if (i != 0)
> >              env->halted = 1;
> > -        qemu_register_reset(main_cpu_reset, 0, env);
> >          if ((env->cpuid_features & CPUID_APIC) || smp_cpus > 1) {
> >              apic_init(env);
> > +        } else {
> > +	    /* APIC reset callback resets cpu */
> 
> That comment should rather go into the previous block (and please remove
> the tab).
> 
> > +            qemu_register_reset((QEMUResetHandler*)cpu_reset, 0, env);
> >          }
> >      }
> >  
> 
> Looks fine to me otherwise. Will give these patches a try later today.
> 
Let me know when you tested it and if no additional fixes needed I'll resend
fixed version.

--
			Gleb.

[Qemu-devel] Re: [PATCH 2/2] Don't register cpu reset handler for cpu with APIC.

[Jan Kiszka]

[-- Attachment #1: Type: text/plain, Size: 2029 bytes --]

Gleb Natapov wrote:
> On Tue, Jun 09, 2009 at 12:21:09PM +0200, Jan Kiszka wrote:
>> Gleb Natapov wrote:
>>> APIC reset handler already resets cpu. Also register cpu_reset handler
>>> directly to make it impossible to add additional code to main_cpu_reset()
>>> by mistake.
>>>
>>> Signed-off-by: Gleb Natapov <gleb@redhat.com>
>>> ---
>>>  hw/pc.c |   10 +++-------
>>>  1 files changed, 3 insertions(+), 7 deletions(-)
>>>
>>> diff --git a/hw/pc.c b/hw/pc.c
>>> index d5b4112..5e7b115 100644
>>> --- a/hw/pc.c
>>> +++ b/hw/pc.c
>>> @@ -751,12 +751,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 };
>>> @@ -878,9 +872,11 @@ static void pc_init1(ram_addr_t ram_size,
>>>          }
>>>          if (i != 0)
>>>              env->halted = 1;
>>> -        qemu_register_reset(main_cpu_reset, 0, env);
>>>          if ((env->cpuid_features & CPUID_APIC) || smp_cpus > 1) {
>>>              apic_init(env);
>>> +        } else {
>>> +	    /* APIC reset callback resets cpu */
>> That comment should rather go into the previous block (and please remove
>> the tab).
>>
>>> +            qemu_register_reset((QEMUResetHandler*)cpu_reset, 0, env);
>>>          }
>>>      }
>>>  
>> Looks fine to me otherwise. Will give these patches a try later today.
>>
> Let me know when you tested it and if no additional fixes needed I'll resend
> fixed version.

No regressions found with both patches applied.

I was not able to convince a 586 Linux kernel to boot more than one CPU
in the absence of LAPICs. But that was independent of the patches and is
likely a BIOS issue ("BIOS bug, local APIC #0 not detected!...").

Jan


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 257 bytes --]