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

[liu ping fan]

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.

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

Hope for suggestion and direct from you, and make a further step for
CPU hot-plug feature,

Thanks and regards,
ping fan


> ...
>>>> diff --git a/hw/icc_bus.c b/hw/icc_bus.c
>>>> new file mode 100644
>>>> index 0000000..360ca2a
>>>> --- /dev/null
>>>> +++ b/hw/icc_bus.c
>>>> @@ -0,0 +1,62 @@
>>>> +/*
>>>> +*/
>>>> +#define ICC_BUS_PLUG
>>>> +#ifdef ICC_BUS_PLUG
>>>> +#include "icc_bus.h"
>>>> +
>>>> +
>>>> +
>>>> +struct icc_bus_info icc_info = {
>>>> +    .qinfo.name = "icc",
>>>> +    .qinfo.size = sizeof(struct icc_bus),
>>>> +    .qinfo.props = (Property[]) {
>>>> +        DEFINE_PROP_END_OF_LIST(),
>>>> +    }
>>>> +
>>>> +};
>>>> +
>>>> +
>>>> +static const VMStateDescription vmstate_icc_bus = {
>>>> +    .name = "icc_bus",
>>>> +    .version_id = 1,
>>>> +    .minimum_version_id = 1,
>>>> +    .minimum_version_id_old = 1,
>>>> +    .pre_save = NULL,
>>>> +    .post_load = NULL,
>>>> +};
>>>> +
>>>> +struct icc_bus *g_iccbus;
>>>> +
>>>> +struct icc_bus *icc_init_bus(DeviceState *parent, const char *name)
>>>> +{
>>>> +    struct icc_bus *bus;
>>>> +
>>>> +    bus = FROM_QBUS(icc_bus, qbus_create(&icc_info.qinfo, parent,
>>> name));
>>>> +    bus->qbus.allow_hotplug = 1; /* Yes, we can */
>>>> +    bus->qbus.name = "icc";
>>>> +    vmstate_register(NULL, -1, &vmstate_icc_bus, bus);
>>>
>>> The chipset is the owner of this bus and instantiates it. So it also
>>> provides a vmstate. You can drop this unneeded one here (it's created
>>> via an obsolete API anyway).
>>>
>>
>> No familiar with Qemu bus emulation, keep on learning :) . But what I
>> thought is,
>> the x86-ICC bus is not the same as bus like PCI.
>> For a PCI bus, it lies behind a host bridge, but ICC is shared by all x86
>> processors in SMP system,
>> so there is not a outstanding owner.  And I right?
>
> ICC is also attached to the chipset (due to the IOAPIC). So it looks
> reasonable to me to let the chipset do the lifecycle management as well.
> It is the fixed point, CPUs may come and go.
>
> Jan
>
>