Re: [Qemu-devel] [PATCH v3 1/2] apic: move apic mmio register to each apic realizefn

["Andreas Färber" <afaerber@suse.de>]

Am 14.04.2015 um 14:41 schrieb Igor Mammedov:
> On Fri, 10 Apr 2015 16:18:05 +0800
> Chen Fan <chen.fan.fnst@cn.fujitsu.com> wrote:
> 
>> Due to local apic address is in view of CPU's address space,
>> so able to move apic mapping to each apic realizefn.
>>
>> Signed-off-by: Chen Fan <chen.fan.fnst@cn.fujitsu.com>
>> ---
>>  hw/i386/pc.c          |  7 -------
>>  hw/intc/apic_common.c | 11 +++++------
>>  2 files changed, 5 insertions(+), 13 deletions(-)
>>
>> diff --git a/hw/i386/pc.c b/hw/i386/pc.c
>> index a8e6be1..a5e2a27 100644
>> --- a/hw/i386/pc.c
>> +++ b/hw/i386/pc.c
>> @@ -1089,13 +1089,6 @@ 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);
>> -    }
>> -
>>      /* tell smbios about cpuid version and features */
>>      smbios_set_cpuid(cpu->env.cpuid_version, cpu->env.features[FEAT_1_EDX]);
>>  }
>> diff --git a/hw/intc/apic_common.c b/hw/intc/apic_common.c
>> index 042e960..004821a 100644
>> --- a/hw/intc/apic_common.c
>> +++ b/hw/intc/apic_common.c
>> @@ -306,7 +306,6 @@ static void apic_common_realize(DeviceState *dev, Error **errp)
>>      APICCommonClass *info;
>>      static DeviceState *vapic;
>>      static int apic_no;
>> -    static bool mmio_registered;
>>  
>>      if (apic_no >= MAX_APICS) {
>>          error_setg(errp, "%s initialization failed.",
>> @@ -317,11 +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);
>> -        mmio_registered = true;
>> -    }
>> +
>> +    memory_region_add_subregion_overlap(CPU(s->cpu)->as->root,
> if you look at AddressSpace declaration, root field is marked as private
> so you can't access it.
> Perhaps you need to add wrapper to get root, something like this:
> address_space_root_memory_region()

Also, by convention please use a local CPUState *cpu or *cs variable
rather than calling CPU() inline.

Regards,
Andreas

>> +                                        APIC_DEFAULT_ADDRESS,
> use s->apicbase here and set its default value from CPU using cpu_set_apic_base()
> for example in: x86_cpu_apic_create()
> 
> PS:
> we also probably need to teach kvm_apic_set_base()/apic_set_base() to
> remap APIC in case base changes but it's out of scope of this series.
> 
>> +                                        &s->io_memory,
>> +                                        0x1000);
>>  
>>      /* Note: We need at least 1M to map the VAPIC option ROM */
>>      if (!vapic && s->vapic_control & VAPIC_ENABLE_MASK &&

-- 
SUSE Linux GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Felix Imendörffer, Jane Smithard, Jennifer Guild, Dilip Upmanyu,
Graham Norton; HRB 21284 (AG Nürnberg)