From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:36013) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1YZyRk-0001O4-JA for qemu-devel@nongnu.org; Mon, 23 Mar 2015 05:15:01 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1YZyRf-0004fc-Uo for qemu-devel@nongnu.org; Mon, 23 Mar 2015 05:14:56 -0400 Received: from [59.151.112.132] (port=7535 helo=heian.cn.fujitsu.com) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1YZyRf-0004fO-HB for qemu-devel@nongnu.org; Mon, 23 Mar 2015 05:14:51 -0400 Message-ID: <550FD7D1.2050307@cn.fujitsu.com> Date: Mon, 23 Mar 2015 17:07:29 +0800 From: Chen Fan MIME-Version: 1.0 References: <20150323092319.7da8cf84@nial.brq.redhat.com> In-Reply-To: <20150323092319.7da8cf84@nial.brq.redhat.com> Content-Type: text/plain; charset="windows-1252"; format=flowed Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH v2 1/2] cpu/apic: drop icc bus/bridge/ List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Igor Mammedov Cc: zhugh.fnst@cn.fujitsu.com, qemu-devel@nongnu.org, tangchen@cn.fujitsu.com, isimatu.yasuaki@jp.fujitsu.com, guz.fnst@cn.fujitsu.com, afaerber@suse.de On 03/23/2015 04:23 PM, Igor Mammedov wrote: > On Mon, 23 Mar 2015 13:54:23 +0800 > Chen Fan wrote: > >> ICC bus was invented only to provide hotplug capability to >> CPU and APIC because at the time being hotplug was available only for >> BUS attached devices. >> >> Now this patch is to drop ICC bus impl, and switch to bus-less >> CPU+APIC hotplug, handling them in the same manner as pc-dimm. >> >> Signed-off-by: Chen Fan >> --- >> hw/i386/pc.c | 29 +++++++++++------------------ >> hw/i386/pc_piix.c | 9 +-------- >> hw/i386/pc_q35.c | 9 +-------- >> hw/intc/apic.c | 6 +++--- >> hw/intc/apic_common.c | 11 ++--------- >> include/hw/i386/apic_internal.h | 5 ++--- >> include/hw/i386/pc.h | 2 +- >> target-i386/cpu.c | 2 -- >> 8 files changed, 21 insertions(+), 52 deletions(-) >> >> diff --git a/hw/i386/pc.c b/hw/i386/pc.c >> index 4b46c29..5d15473 100644 >> --- a/hw/i386/pc.c >> +++ b/hw/i386/pc.c > [...] > >> @@ -1093,8 +1083,11 @@ void pc_cpus_init(const char *cpu_model, DeviceState *icc_bridge) >> /* map APIC MMIO area if CPU has APIC */ >> if (cpu && cpu->apic_state) { >> /* XXX: what if the base changes? */ >> - sysbus_mmio_map_overlap(SYS_BUS_DEVICE(icc_bridge), 0, >> - APIC_DEFAULT_ADDRESS, 0x1000); >> + apic = APIC_COMMON(cpu->apic_state); >> + memory_region_add_subregion_overlap(CPU(cpu)->as->root, >> + APIC_DEFAULT_ADDRESS, >> + &apic->io_memory, >> + 0x1000); > Why is it here? > Shouldn't it be mapped not once but for each CPU since we are using > per CPU address spaces? > > Split this change out into a separate patch please, with commit message > describing what it does. Hi Igor, in your previous mail said, "It might be that kvm_irqchip don't need it at all." I don't know why kvm_irqchip don't need it ? because I have test that for kernel_irqchip=on, qemu emulator the kvm-apic object, and sent the MSI to kernel irqchip for pcie devices. it also need map the region. Thanks, Chen > > PS: > It should be part of APIC code or at worst case part of CPU's realize. > > PS2: > new cpu tests don't test actual CPU execution, so they can't validate > this change. To test it you need to run test in TCG (at least) or > TCG + KVM mode, with some guest code that programs and checks APIC > of each CPU. > > PS3: > the rest of the patch I'd suggest to merge with 2/2 patch that > removes unused icc_bridge code, there isn't point in splitting > that from removing icc_bridge from other files. > > [...] >> >> diff --git a/target-i386/cpu.c b/target-i386/cpu.c >> index f01690b..2385e6b 100644 >> --- a/target-i386/cpu.c >> +++ b/target-i386/cpu.c >> @@ -42,7 +42,6 @@ >> >> #include "sysemu/sysemu.h" >> #include "hw/qdev-properties.h" >> -#include "hw/cpu/icc_bus.h" >> #ifndef CONFIG_USER_ONLY >> #include "hw/xen/xen.h" >> #include "hw/i386/apic_internal.h" >> @@ -2941,7 +2940,6 @@ static void x86_cpu_common_class_init(ObjectClass *oc, void *data) >> >> xcc->parent_realize = dc->realize; >> dc->realize = x86_cpu_realizefn; >> - dc->bus_type = TYPE_ICC_BUS; > that isn't the only place in this file that should be changed. > > See x86_cpu_apic_create(): > cpu->apic_state = qdev_try_create(qdev_get_parent_bus(dev), apic_type); > > probably it's not right to try get parent bus from bus-less device, > qdev_try_create() call should be replaced by object_new()/object_unref() pair. > >> dc->props = x86_cpu_properties; >> >> xcc->parent_reset = cc->reset; > . >