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

[Eduardo Habkost]

On Wed, Aug 19, 2015 at 05:36:29PM +0800, Zhu Guihua wrote:
> From: Chen Fan <address@hidden>
> 
> Replace mapping APIC at global system address space with
> mapping it at per-CPU address spaces.

Can you improve the commit message by explaining not just what is being
done, but why this is needed?

> 
> Signed-off-by: Chen Fan <address@hidden>
> Signed-off-by: Zhu Guihua <address@hidden>
> ---
>  hw/i386/pc.c          |  7 -------
>  hw/intc/apic_common.c |  6 ------
>  target-i386/cpu.c     | 21 +++++++++++++++++++++
>  3 files changed, 21 insertions(+), 13 deletions(-)
> 
> diff --git a/hw/i386/pc.c b/hw/i386/pc.c
> index 9f2924e..8b7dbe5 100644
> --- a/hw/i386/pc.c
> +++ b/hw/i386/pc.c
> @@ -1158,13 +1158,6 @@ void pc_cpus_init(const char *cpu_model, 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..8eed88c 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,8 +2790,10 @@ 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;
> +    static bool apic_mmio_map_once;
>  
>      if (cpu->apic_id < 0) {
>          error_setg(errp, "apic-id property was not initialized properly");
> @@ -2877,6 +2880,24 @@ static void x86_cpu_realizefn(DeviceState *dev, Error 
> **errp)
>      if (local_err != NULL) {
>          goto out;
>      }
> +
> +    /* map APIC MMIO area */
> +    apic = APIC_COMMON(cpu->apic_state);
> +    if (tcg_enabled()) {
> +        memory_region_add_subregion_overlap(cpu->cpu_as_root,
> +                                            apic->apicbase &
> +                                            MSR_IA32_APICBASE_BASE,
> +                                            &apic->io_memory,
> +                                            0x1000);

Why exactly is this necessary? If this is necessary, why don't we need
to do this for non-TCG accelerators?


> +    } else if (!apic_mmio_map_once) {
> +        memory_region_add_subregion_overlap(get_system_memory(),
> +                                            apic->apicbase &
> +                                            MSR_IA32_APICBASE_BASE,
> +                                            &apic->io_memory,
> +                                            0x1000);
> +        apic_mmio_map_once = true;
> +    }

I see that you are doing two things at the same time:
1) Moving the memory region registration to x86_cpu_realizefn();
2) Adding a special case for TCG that uses cpu->cpu_as_root.

Doing this in two separate patches seems more appropriate.

-- 
Eduardo