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

[Zhu Guihua]

On 08/31/2015 09:21 PM, Igor Mammedov wrote:
> On Mon, 31 Aug 2015 17:47:44 +0800
> Zhu Guihua <address@hidden> wrote:
>
> > From: Chen Fan <address@hidden>
> >
> > 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 <address@hidden>
> > Signed-off-by: Zhu Guihua <address@hidden>
> > ---
> >   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);
> .