From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:49962) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Yd7e9-0006ef-7z for qemu-devel@nongnu.org; Tue, 31 Mar 2015 21:40:51 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1Yd7e5-0005Jp-Qy for qemu-devel@nongnu.org; Tue, 31 Mar 2015 21:40:45 -0400 Received: from [59.151.112.132] (port=31139 helo=heian.cn.fujitsu.com) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Yd7e4-0005IW-KN for qemu-devel@nongnu.org; Tue, 31 Mar 2015 21:40:41 -0400 Message-ID: <551B4C80.7090803@cn.fujitsu.com> Date: Wed, 1 Apr 2015 09:40:16 +0800 From: Chen Fan MIME-Version: 1.0 References: <20150323092319.7da8cf84@nial.brq.redhat.com> <550FD7D1.2050307@cn.fujitsu.com> <20150323104328.62d2910b@nial.brq.redhat.com> <55192176.5060904@cn.fujitsu.com> <20150330144547.70e6589c@nial.brq.redhat.com> <551A60D3.2090704@cn.fujitsu.com> <20150331115103.3a9d39c7@igors-macbook-pro.local> In-Reply-To: <20150331115103.3a9d39c7@igors-macbook-pro.local> 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/31/2015 05:51 PM, Igor Mammedov wrote: > On Tue, 31 Mar 2015 16:54:43 +0800 > Chen Fan wrote: > >> On 03/30/2015 08:45 PM, Igor Mammedov wrote: >>> On Mon, 30 Mar 2015 18:12:06 +0800 >>> Chen Fan wrote: >>> >>>> On 03/23/2015 05:43 PM, Igor Mammedov wrote: >>>>> On Mon, 23 Mar 2015 17:07:29 +0800 >>>>> Chen Fan wrote: >>>>> >>>>>> 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 ? >>>>> That's why it's MIGHT, I'm not sure since I've not look at that >>>>> code for a while. >>>>> >>>>>> 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. >>>>> Can we have this test as a patch to qemu/tests? so it would be >>>>> easier to discuss it. >>>> kernel_irqchip is only used for kvm acc, do qtest can use kvm >>>> accel? >>> As far as I know it can't by I might be wrong. >>> >>> I'd suggest execute tests with accel=tcg and conditionally with >>> accel=kvm if KVM on host is available and accessible to make check >>> user. >> I hadn't found a good way to test this case, do you any idea? > maybe check that /dev/kvm exists and accessible to user, if yes than > run additional tests with accel=kvm I mean that how to make the tests concretely can check the case that when kernel_irqchip on also need kvm-apic mapped ? Thanks, Chen >> Thanks, >> Chen >> >>>> I used GDB to intercept the kvm_apic_mem_write(), we could find >>>> that: >>>> >>>> #0 kvm_apic_mem_write (opaque=0x55555652ddb0, addr=0, data=16465, >>>> size=4) at /home/chenfan/data/qemu-latest/hw/i386/kvm/apic.c:157 >>>> #1 0x000055555565c871 in memory_region_write_accessor >>>> (mr=0x55555652de28, addr=0, value=0x7fffe5027538, size=4, shift=0, >>>> mask=4294967295) >>>> at /home/chenfan/data/qemu-latest/memory.c:430 >>>> #2 0x000055555565c9b9 in access_with_adjusted_size (addr=0, >>>> value=0x7fffe5027538, size=4, access_size_min=1, >>>> access_size_max=4, access= 0x55555565c7d9 >>>> , mr=0x55555652de28) >>>> at /home/chenfan/data/qemu-latest/memory.c:467 #3 >>>> 0x000055555565f9d1 in memory_region_dispatch_write >>>> (mr=0x55555652de28, addr=0, data=16465, size=4) >>>> at /home/chenfan/data/qemu-latest/memory.c:1103 #4 >>>> 0x000055555566356e in io_mem_write (mr=0x55555652de28, addr=0, >>>> val=16465, size=4) at /home/chenfan/data/qemu-latest/memory.c:2003 >>>> #5 0x00005555556060f2 in stl_phys_internal (as=0x5555577568a8, >>>> addr=4276092928, val=16465, endian=DEVICE_LITTLE_ENDIAN) #6 >>>> 0x000055555560621e in stl_le_phys (as=0x5555577568a8, >>>> addr=4276092928, val=16465) >>>> at /home/chenfan/data/qemu-latest/exec.c:2920 #7 >>>> 0x000055555587d35e in *msi_notify* (dev=0x5555577566a0, vector=0) >>>> at hw/pci/msi.c:294 #8 0x0000555555836f77 in ahci_irq_raise >>>> (s=0x555557756f20, dev=0x0) at hw/ide/ahci.c:134 #9 >>>> 0x00005555558370f2 in ahci_check_irq (s=0x555557756f20) at >>>> hw/ide/ahci.c:169 #10 0x000055555583733a in ahci_port_write >>>> (s=0x555557756f20, port=0, offset=20, val=2017460351) at >>>> hw/ide/ahci.c:225 #11 0x0000555555837811 in ahci_mem_write >>>> (opaque=0x555557756f20, addr=276, val=2017460351, size=4) at >>>> hw/ide/ahci.c:382 #12 0x000055555565c871 in >>>> memory_region_write_accessor (mr=0x555557756f40, addr=276, >>>> value=0x7fffe50278b8, size=4, shift=0, mask=4294967295) >>>> at /home/chenfan/data/qemu-latest/memory.c:430 >>>> #13 0x000055555565c9b9 in access_with_adjusted_size (addr=276, >>>> value=0x7fffe50278b8, size=4, access_size_min=1, >>>> access_size_max=4, access= 0x55555565c7d9 >>>> , mr=0x555557756f40) >>>> at /home/chenfan/data/qemu-latest/memory.c:467 #14 >>>> 0x000055555565f9d1 in memory_region_dispatch_write >>>> (mr=0x555557756f40, addr=276, data=2017460351, size=4) >>>> at /home/chenfan/data/qemu-latest/memory.c:1103 #15 >>>> 0x000055555566356e in io_mem_write (mr=0x555557756f40, addr=276, >>>> val=2017460351, size=4) >>>> at /home/chenfan/data/qemu-latest/memory.c:2003 >>>> >>>> >>>> >>>> Thanks, >>>> Chen >>>> >>>> >>>>>> 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; >>>>>>> . >>>>>>> >>>>> . >>>>> >>> . >>> >> > . >