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

[Jan Kiszka]

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.

Jan

[1] http://thread.gmane.org/gmane.comp.emulators.qemu/100806

signature.asc
Description: OpenPGP digital signature