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

[Jan Kiszka <jan.kiszka@web.de>]

[-- Attachment #1: Type: text/plain, Size: 3453 bytes --]

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.

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

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


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 262 bytes --]