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 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.
>
> 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.
Hi Igor,

when I tested for this, use object_new() to replace 
qdev_try_create(qdev_get_parent_bus(dev), apic_type),
run with tcg, it works OK, but if run with kvm, it failed. did apic must 
need sysbus?

Thanks,
Chen


>
>>       dc->props = x86_cpu_properties;
>>   
>>       xcc->parent_reset = cc->reset;
> .
>