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

[Jan Kiszka <jan.kiszka@siemens.com>]

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

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.

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

Jan

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