From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:52936) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Yd7kz-0003rh-KY for qemu-devel@nongnu.org; Tue, 31 Mar 2015 21:47:54 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1Yd7ku-00089j-TH for qemu-devel@nongnu.org; Tue, 31 Mar 2015 21:47:49 -0400 Received: from [59.151.112.132] (port=46724 helo=heian.cn.fujitsu.com) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Yd7kr-00087R-HX for qemu-devel@nongnu.org; Tue, 31 Mar 2015 21:47:44 -0400 Message-ID: <551B4E2E.1080202@cn.fujitsu.com> Date: Wed, 1 Apr 2015 09:47:26 +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> <551B4C80.7090803@cn.fujitsu.com> In-Reply-To: <551B4C80.7090803@cn.fujitsu.com> Content-Type: multipart/alternative; boundary="------------040603080606030502050405" 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 --------------040603080606030502050405 Content-Type: text/plain; charset="UTF-8"; format=flowed Content-Transfer-Encoding: 7bit On 04/01/2015 09:40 AM, Chen Fan wrote: > > 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 ? for pcie supported MSI, the MSI Message Adress is usually based on 0xFEEX-XXXX, which is the same as APIC mmio base address on x86. so I can think is to send MSI by write the address space then check whether could intercept the msi irq. > > 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; >>>>>>>> . >>>>>>>> >>>>>> . >>>>>> >>>> . >>>> >>> >> . >> > > > . > --------------040603080606030502050405 Content-Type: text/html; charset="UTF-8" Content-Transfer-Encoding: 8bit
On 04/01/2015 09:40 AM, Chen Fan wrote:

On 03/31/2015 05:51 PM, Igor Mammedov wrote:
On Tue, 31 Mar 2015 16:54:43 +0800
Chen Fan <chen.fan.fnst@cn.fujitsu.com> wrote:

On 03/30/2015 08:45 PM, Igor Mammedov wrote:
On Mon, 30 Mar 2015 18:12:06 +0800
Chen Fan <chen.fan.fnst@cn.fujitsu.com> wrote:

On 03/23/2015 05:43 PM, Igor Mammedov wrote:
On Mon, 23 Mar 2015 17:07:29 +0800
Chen Fan <chen.fan.fnst@cn.fujitsu.com> wrote:

On 03/23/2015 04:23 PM, Igor Mammedov wrote:
On Mon, 23 Mar 2015 13:54:23 +0800
Chen Fan <chen.fan.fnst@cn.fujitsu.com> 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 <chen.fan.fnst@cn.fujitsu.com>
---
     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 ?
for pcie supported MSI, the MSI Message Adress is usually based on 0xFEEX-XXXX,
which is the same as APIC mmio base address on x86. so I can think is
to send MSI by write the address space then check whether could intercept the msi irq.


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
<memory_region_write_accessor>, 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
<memory_region_write_accessor>, 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;
.

.

.


.



.


--------------040603080606030502050405--