From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:35269) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1YyPwF-0003q8-RI for qemu-devel@nongnu.org; Fri, 29 May 2015 15:27:28 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1YyPwC-00010d-Kq for qemu-devel@nongnu.org; Fri, 29 May 2015 15:27:27 -0400 Received: from mx1.redhat.com ([209.132.183.28]:44195) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1YyPwC-00010Y-Gk for qemu-devel@nongnu.org; Fri, 29 May 2015 15:27:24 -0400 Message-ID: <5568BD97.8030703@redhat.com> Date: Fri, 29 May 2015 21:27:19 +0200 From: Paolo Bonzini MIME-Version: 1.0 References: <1432922664-15129-1-git-send-email-ehabkost@redhat.com> <1432922664-15129-3-git-send-email-ehabkost@redhat.com> In-Reply-To: <1432922664-15129-3-git-send-email-ehabkost@redhat.com> Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PULL 2/5] 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: Eduardo Habkost , qemu-devel@nongnu.org, Peter Maydell Cc: Chen Fan , =?UTF-8?B?QW5kcmVhcyBGw6RyYmVy?= , Richard Henderson On 29/05/2015 20:04, Eduardo Habkost wrote: > static int apic_no; > - static bool mmio_registered; > + CPUState *cpu = CPU(s->cpu); > + MemoryRegion *root; > > if (apic_no >= MAX_APICS) { > error_setg(errp, "%s initialization failed.", > @@ -307,11 +308,12 @@ 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; > - } > + > + root = address_space_root_memory_region(cpu->as); I think just using cpu->as->root is okay. > + memory_region_add_subregion_overlap(root, > + s->apicbase & MSR_IA32_APICBASE_BASE, > + &s->io_memory, > + 0x1000); I think this patch is incorrect, because you do not install a separate address space for each CPU. Also, the CPU address space is only used with TCG so it should be guarded by "if (tcg_enabled())". Paolo > /* Note: We need at least 1M to map the VAPIC option ROM */ > if (!vapic && s->vapic_control & VAPIC_ENABLE_MASK && > diff --git a/include/exec/memory.h b/include/exec/memory.h > index b61c84f..a16650f 100644 > --- a/include/exec/memory.h > +++ b/include/exec/memory.h > @@ -1295,6 +1295,11 @@ void *address_space_map(AddressSpace *as, hwaddr addr, > void address_space_unmap(AddressSpace *as, void *buffer, hwaddr len, > int is_write, hwaddr access_len); > > +/* address_space_root_memory_region: get root memory region > + * > + * @as: #AddressSpace to be accessed > + */ > +MemoryRegion *address_space_root_memory_region(AddressSpace *as); > > #endif > > diff --git a/target-i386/cpu.c b/target-i386/cpu.c > index 3305e09..f83e526 100644 > --- a/target-i386/cpu.c > +++ b/target-i386/cpu.c > @@ -2740,6 +2740,8 @@ static void x86_cpu_apic_create(X86CPU *cpu, Error **errp) > /* TODO: convert to link<> */ > apic = APIC_COMMON(cpu->apic_state); > apic->cpu = cpu; > + cpu_set_apic_base(cpu->apic_state, > + APIC_DEFAULT_ADDRESS | MSR_IA32_APICBASE_ENABLE); > } >