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

[Gleb Natapov]

On Tue, Jun 09, 2009 at 10:37:53AM +0200, Jan Kiszka wrote:
> Gleb Natapov wrote:
> > 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 <address@hidden>
> >>> 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.
> 
> The order parameter is obsolete, I already posted a patch to revert this
> idea again (reminds me of posting an update as a new reset handler
> arrived in the meantime).
> 
> The system-wide assumed and applied order is that earlier instantiated
> devices will be reset first. That specifically makes sense if you think
> of bus/device relations.
> 
Sounds fragile to me, but explicit order far from ideal too. Why should
order matter anyway. Device reset callbacks should not rely on the state
of other devices. 

> > On the other hand apic
> > reset handler already resets cpu so if apic is present there is no need to
> > register main_cpu_reset().
> 
> OK, this is a special case as the APIC reset triggers an init IPI and
> that resets the CPU, too. Then make this explicit, replace
> main_cpu_reset with cpu_reset (so that no one adds code to the former
> that is not run in the APIC case) and add some comment why.
Already doing it on top of this patch.

--
                        Gleb.