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

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

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.
no problem.

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