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

[Igor Mammedov]

On Mon, 30 Mar 2015 18:12:06 +0800
Chen Fan <address@hidden> wrote:

> 
> On 03/23/2015 05:43 PM, Igor Mammedov wrote:
> > On Mon, 23 Mar 2015 17:07:29 +0800
> > Chen Fan <address@hidden> wrote:
> >
> >> On 03/23/2015 04:23 PM, Igor Mammedov wrote:
> >>> On Mon, 23 Mar 2015 13:54:23 +0800
> >>> Chen Fan <address@hidden> 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 <address@hidden>
> >>>> ---
> >>>>    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 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;
> >>> .
> >>>
> > .
> >
> 
>