Re: [Qemu-devel] [PATCH v2 1/2] cpu/apic: drop icc bus/bridge/

[Chen Fan <chen.fan.fnst@cn.fujitsu.com>]

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


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


[-- Attachment #2: Type: text/html, Size: 18544 bytes --]