[Qemu-devel] Re: [PATCH v2] Apic creation should not depend on pci

[Gleb Natapov <gleb@redhat.com>]

On Mon, Jun 08, 2009 at 04:03:19PM +0200, Jan Kiszka wrote:
> Gleb Natapov wrote:
> >     
> > It should depend on whether cpu has APIC.
> > 
> > Signed-off-by: Gleb Natapov <gleb@redhat.com>
> > diff --git a/hw/pc.c b/hw/pc.c
> > index 0934778..cb49772 100644
> > --- a/hw/pc.c
> > +++ b/hw/pc.c
> > @@ -878,14 +878,10 @@ 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) {
> 
> Obviously :), I'm fine with that change. Needs testing, though. What
> scenarios did you already check?
> 
> >              apic_init(env);
> >          }
> > +        qemu_register_reset(main_cpu_reset, 0, env);
> 
> But this line silently reorders CPU and APIC reset handlers. If you did
> it intentionally (I vaguely recall it may have some benefit /wrt KVM
> synchronizing kernel and user space states), I would suggest pushing it
> as a separate patch.
> 
BTW relying on order of callback registration is not a good idea
especially since we have "order" parameter now. On the other hand apic
reset handler already resets cpu so if apic is present there is no need to
register main_cpu_reset().

--
			Gleb.