Re: [Qemu-devel] [PATCH v10 1/4] apic: map APIC's MMIO region at each CPU's address space

[Zhu Guihua <zhugh.fnst@cn.fujitsu.com>]

On 08/31/2015 09:21 PM, Igor Mammedov wrote:
> On Mon, 31 Aug 2015 17:47:44 +0800
> Zhu Guihua <zhugh.fnst@cn.fujitsu.com> wrote:
>
>> From: Chen Fan <chen.fan.fnst@cn.fujitsu.com>
>>
>> After ICC bus/bridge have been removed, APIC MMIO area could
>> not be mapped into sysbus MMIO any more.
>> So replace mapping APIC at global system address space with
>> mapping it at per-CPU address spaces.
> When ICC bus/bridge is removed, APIC MMIO will be left
> unmapped since it was mapped into system's address space
> indirectly by ICC bridge.
> Fix it by moving mapping into APIC code, so it would be
> possible to remove ICC bus/bridge code later.
>
>> Signed-off-by: Chen Fan <chen.fan.fnst@cn.fujitsu.com>
>> Signed-off-by: Zhu Guihua <zhugh.fnst@cn.fujitsu.com>
>> ---
>>   hw/i386/pc.c          |  7 -------
>>   hw/intc/apic_common.c |  6 ------
>>   target-i386/cpu.c     | 16 ++++++++++++++++
>>   3 files changed, 16 insertions(+), 13 deletions(-)
>>
>> diff --git a/hw/i386/pc.c b/hw/i386/pc.c
>> index b1c96a8..e15971c 100644
>> --- a/hw/i386/pc.c
>> +++ b/hw/i386/pc.c
>> @@ -1157,13 +1157,6 @@ void pc_cpus_init(PCMachineState *pcms, DeviceState *icc_bridge)
>>           object_unref(OBJECT(cpu));
>>       }
>>   
>> -    /* 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 0032b97..c0b32eb 100644
>> --- a/hw/intc/apic_common.c
>> +++ b/hw/intc/apic_common.c
>> @@ -296,7 +296,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.",
>> @@ -307,11 +306,6 @@ 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;
>> -    }
>>   
>>       /* Note: We need at least 1M to map the VAPIC option ROM */
>>       if (!vapic && s->vapic_control & VAPIC_ENABLE_MASK &&
>> diff --git a/target-i386/cpu.c b/target-i386/cpu.c
>> index cfb8aa7..171cdc0 100644
>> --- a/target-i386/cpu.c
>> +++ b/target-i386/cpu.c
>> @@ -2745,6 +2745,7 @@ static void x86_cpu_apic_create(X86CPU *cpu, Error **errp)
>>       /* TODO: convert to link<> */
>>       apic = APIC_COMMON(cpu->apic_state);
>>       apic->cpu = cpu;
>> +    apic->apicbase = APIC_DEFAULT_ADDRESS | MSR_IA32_APICBASE_ENABLE;
>>   }
>>   
>>   static void x86_cpu_apic_realize(X86CPU *cpu, Error **errp)
>> @@ -2789,6 +2790,7 @@ static void x86_cpu_realizefn(DeviceState *dev, Error **errp)
>>       X86CPU *cpu = X86_CPU(dev);
>>       X86CPUClass *xcc = X86_CPU_GET_CLASS(dev);
>>       CPUX86State *env = &cpu->env;
>> +    APICCommonState *apic;
>>       Error *local_err = NULL;
>>       static bool ht_warned;
>>   
>> @@ -2877,6 +2879,20 @@ static void x86_cpu_realizefn(DeviceState *dev, Error **errp)
>>       if (local_err != NULL) {
>>           goto out;
>>       }
>> +
>> +    apic = APIC_COMMON(cpu->apic_state);
>> +    /* Map APIC MMIO area, use per-CPU address space if available (TCG
>> +     * supports it, KVM doesn't). This allows the APIC base address of
>> +     * each CPU to be moved independently.
>> +     */
>> +    memory_region_add_subregion_overlap(cpu->cpu_as_root ?
>> +                                        cpu->cpu_as_root :
>> +                                        get_system_memory(),
>> +                                        apic->apicbase &
>> +                                        MSR_IA32_APICBASE_BASE,
>> +                                        &apic->io_memory,
>> +                                        0x1000);
> You have lost apic_mmio_map_once from previous version,
> as result in KVM case io_memory will be mapped many times
> over itself, pls fix it.

I'm sorry to forget apic_mmio_map_once, so the logic must be as previous
version. I will fix it.

Thanks,
Zhu

> Pls, also split per CPU AS change into a separate patch
> as Eduardo asked you for, with following order:
>    1. move region mapping into APIC in this patch
>    2. add per CPU AS in an additional path
>
>>       cpu_reset(cs);
>>   
>>       xcc->parent_realize(dev, &local_err);
> .
>