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

[Igor Mammedov <imammedo@redhat.com>]

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.

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