From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([140.186.70.92]:58292) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1RBd6C-0001RH-Fi for qemu-devel@nongnu.org; Wed, 05 Oct 2011 21:50:13 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1RBd6A-0001IV-RZ for qemu-devel@nongnu.org; Wed, 05 Oct 2011 21:50:12 -0400 Received: from e2.ny.us.ibm.com ([32.97.182.142]:37749) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1RBd6A-0001IR-JK for qemu-devel@nongnu.org; Wed, 05 Oct 2011 21:50:10 -0400 Received: from /spool/local by us.ibm.com with XMail ESMTP for from ; Wed, 5 Oct 2011 21:50:09 -0400 Received: from d01av04.pok.ibm.com (d01av04.pok.ibm.com [9.56.224.64]) by d01relay02.pok.ibm.com (8.13.8/8.13.8/NCO v10.0) with ESMTP id p961o4tX414252 for ; Wed, 5 Oct 2011 21:50:05 -0400 Received: from d01av04.pok.ibm.com (loopback [127.0.0.1]) by d01av04.pok.ibm.com (8.14.4/8.13.1/NCO v10.0 AVout) with ESMTP id p961o3CF020114 for ; Wed, 5 Oct 2011 21:50:04 -0400 Message-ID: <4E8D0948.30302@us.ibm.com> Date: Wed, 05 Oct 2011 20:50:00 -0500 From: Anthony Liguori MIME-Version: 1.0 References: <1317726818-8514-1-git-send-email-pingfank@linux.vnet.com> <1317726818-8514-4-git-send-email-pingfank@linux.vnet.com> <4E8B3794.9040301@web.de> <4E8C38F3.2070504@web.de> In-Reply-To: Content-Type: text/plain; charset=windows-1252; format=flowed Content-Transfer-Encoding: 8bit Subject: Re: [Qemu-devel] [PATCH 2/2] LAPIC: make lapic support cpu hotplug List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: liu ping fan Cc: ryanh@linux.vnet.ibm.com, pingfank@linux.vnet.ibm.com, kvm@vger.kernel.org, qemu-devel@nongnu.org, Jan Kiszka , avi@redhat.com On 10/05/2011 08:13 PM, liu ping fan wrote: > On Wed, Oct 5, 2011 at 7:01 PM, Jan Kiszka 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 >> >> >