From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:59053) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ZWa21-0003D0-KL for qemu-devel@nongnu.org; Mon, 31 Aug 2015 21:06:39 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1ZWa1w-0006NV-U3 for qemu-devel@nongnu.org; Mon, 31 Aug 2015 21:06:37 -0400 Received: from [59.151.112.132] (port=38655 helo=heian.cn.fujitsu.com) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ZWa1w-0006NO-3Q for qemu-devel@nongnu.org; Mon, 31 Aug 2015 21:06:32 -0400 Message-ID: <55E4F9C5.2000701@cn.fujitsu.com> Date: Tue, 1 Sep 2015 09:05:09 +0800 From: Zhu Guihua MIME-Version: 1.0 References: <52984e6608000b4916a456a9c27d0bf5f68d3bf1.1441008774.git.zhugh.fnst@cn.fujitsu.com> <20150831152136.79b4dd4a@nial.brq.redhat.com> In-Reply-To: <20150831152136.79b4dd4a@nial.brq.redhat.com> Content-Type: text/plain; charset="windows-1252"; format=flowed Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH v10 1/4] apic: map APIC's MMIO region at each CPU's address space List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Igor Mammedov Cc: ehabkost@redhat.com, qemu-devel@nongnu.org, izumi.taku@jp.fujitsu.com, chen.fan.fnst@cn.fujitsu.com, pbonzini@redhat.com, afaerber@suse.de On 08/31/2015 09:21 PM, Igor Mammedov wrote: > On Mon, 31 Aug 2015 17:47:44 +0800 > Zhu Guihua wrote: > >> From: Chen Fan >> >> 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 >> Signed-off-by: Zhu Guihua >> --- >> 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); > . >