Re: [Qemu-devel] [PATCH 2/2] LAPIC: make lapic support cpu hotplug

[liu ping fan]

On 10/6/11, Jan Kiszka <address@hidden> wrote:
> On 2011-10-06 03:13, liu ping fan wrote:
>> On Wed, Oct 5, 2011 at 7:01 PM, Jan Kiszka <address@hidden> wrote:
>>> On 2011-10-05 12:26, liu ping fan wrote:
>>>>>  > And make the creation of apic as part of cpu initialization, so
>>>>>> apic's state has been ready, before setting kvm_apic.
>>>>>
>>>>> There is no kvm-apic upstream yet, so it's hard to judge why we need
>>>>> this here. If we do, this has to be a separate patch. But I seriously
>>>>> doubt we need it (my hack worked without it, and that was not because
>>>>> of
>>>>> its hack nature).
>>>>>
>>>>> Sorry, I did not explain it clearly. What I mean is that
>>>>> “env->apic_state”
>>>> must be prepared
>>>> before qemu_kvm_cpu_thread_fn() -> ... -> kvm_put_sregs(), where we get
>>>> apic_base by
>>>> “ sregs.apic_base = cpu_get_apic_base(env->apic_state);”
>>>> and then call “kvm_vcpu_ioctl(env, KVM_SET_SREGS, &sregs);” which will
>>>> finally affect the
>>>> kvm_apic structure in kernel.
>>>>
>>>> But as current code, in pc_new_cpu(), we call apic_init() to initialize
>>>> apic_state, after cpu_init(),
>>>> so we can not guarantee the order of apic_state initializaion and the
>>>> setting to kernel.
>>>>
>>>> Because LAPIC is part of x86 chip, I want to move it into
>>>> cpu_x86_init(),
>>>> and ensure apic_init()
>>>> called before thread “qemu_kvm_cpu_thread_fn()” creation.
>>>
>>> The LAPIC is part of the CPU, the classic APIC was a dedicated chip.
>> Sorry, a little puzzle.  I think x86 interrupt system consists of two
>> parts: IOAPIC/LAPIC.
>> For we have "hw/ioapic.c" to simulate IOAPIC,  I think "hw/apic.c"
>> takes the role as LAPIC,
>> especially that we create an APICState instance for each CPUX86State,
>> just like each LAPIC
>> for x86 CPU in real machine.
>> So we can consider apic_init() to create a LAPIC instance, rather than
>> create a  "classic APIC"?
>>
>> I guess If there is lack of something in IOAPIC/LAPIC bus topology,
>> that will be the arbitrator of ICC bus, right?
>> So "the classic APIC was a dedicated chip" what you said, play this
>> role,  right?
>> Could you tell me a sample chipset of APIC, and I can increase my
>> knowledge about it, thanks.
>
> The 82489DX was used as a discrete APIC on 486 SMP systems.
>
>>
>>>
>>> For various reasons, a safer approach for creating a new CPU is to stop
>>> the machine, add the new device models, run cpu_synchronize_post_init on
>>> that new cpu (looks like you missed that) and then resume everything.
>>> See
>>> http://git.kiszka.org/?p=qemu-kvm.git;a=commitdiff;h=be8f21c6b54eac82f7add7ee9d4ecf9cb8ebb320
>>>
>> Great job. And I am interesting about it. Could you give some sample
>> reason about why we need to stop
>> the machine, or list all of the reasons, so we can resolve it one by
>> one. I can not figure out such scenes by myself.
>> From my view, especially for KVM, the creation of vcpu are protected
>> well by lock mechanism from other
>> vcpu threads in kernel, so we need not to stop all of the threads.
>
> Maybe I was seeing ghosts: I thought that there is a race window between
> VCPU_CREATE and the last initialization IOCTL when we allow other VCPUs
> to interact with the new one already. However, I do not find the
> scenario again ATM.
>
> But if you want to move the VCPU resource initialization completely over
> the VCPU thread, you also have to handle env->halted in that context.
> See [1] for this topic and associated todos.
>
> And don't forget the cpu_synchronize_post_init. Running this after each
> VCPU creation directly should also obsolete cpu_synchronize_all_post_init.
Thanks, Jan.  I will dig into this and follow the thread to see what
to do in next
step

Regards,
ping fan
>
> Jan
>
> [1] http://thread.gmane.org/gmane.comp.emulators.qemu/100806
>
>