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

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

On 03/18/2015 12:29 AM, Igor Mammedov wrote:
> On Tue, 17 Mar 2015 17:08:30 +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>
>> ---
> [...]
>
>> diff --git a/hw/intc/apic_common.c b/hw/intc/apic_common.c
>> index 0858b45..020219e 100644
>> --- a/hw/intc/apic_common.c
>> +++ b/hw/intc/apic_common.c
>> @@ -27,6 +27,8 @@
>>   static int apic_irq_delivered;
>>   bool apic_report_tpr_access;
>>   
>> +static MemoryRegion *apic_container;
>> +
>>   void cpu_set_apic_base(DeviceState *dev, uint64_t val)
>>   {
>>       trace_cpu_set_apic_base(val);
>> @@ -314,8 +316,11 @@ static void apic_common_realize(DeviceState *dev, Error **errp)
>>       info = APIC_COMMON_GET_CLASS(s);
>>       info->realize(dev, errp);
>>       if (!mmio_registered) {
>> -        ICCBus *b = ICC_BUS(qdev_get_parent_bus(dev));
>> -        memory_region_add_subregion(b->apic_address_space, 0, &s->io_memory);
>> +        memory_region_add_subregion(apic_container, 0, &s->io_memory);
>> +        memory_region_add_subregion_overlap(CPU(s->cpu)->as->root,
>> +                                            APIC_DEFAULT_ADDRESS,
>> +                                            apic_container,
>> +                                            0x1000);
>>           mmio_registered = true;
>>       }
> Provided per CPU address spaces are used,
> why do you need static apic_container and mmio_registered thing?
> Shouldn't it be possible to register &s->io_memory of each APIC
> in CPU's own AS and avoid using globals?
it seems only need to do
memory_region_add_subregion_overlap(CPU(s->cpu)->as->root,
                                             APIC_DEFAULT_ADDRESS,
                                             &io_memory,
                                             0x1000);

at here, right?

Thanks,
Chen
>
>>   
>> @@ -431,15 +436,23 @@ static Property apic_properties_common[] = {
>>       DEFINE_PROP_END_OF_LIST(),
>>   };
>>   
>> +static void apic_common_initfn(Object *obj)
>> +{
>> +    if (!apic_container) {
>> +        apic_container = g_malloc(sizeof(*apic_container));
>> +        memory_region_init(apic_container, obj, "apic-container",
>> +                           APIC_SPACE_SIZE);
>> +    }
>> +}
>> +
>
>
> .
>