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

[Anthony Liguori <aliguori@us.ibm.com>]

On 10/05/2011 08:13 PM, liu ping fan wrote:
> On Wed, Oct 5, 2011 at 7:01 PM, Jan Kiszka<jan.kiszka@web.de>  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.

I think Jan meant the PIC was a dedicated chip.  hw/apic.c implements an LAPIC, 
hw/i8259.c implements an I8259A otherwise known as the PIC.  hw/ioapic.c 
implements an I/O APIC.

Together, the I/O APIC and the LAPIC form an 'APIC Architecture'.  Usually, the 
legacy PIC can hang off of the BSP LAPIC.

Regards,

Anthony Liguori

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