From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([208.118.235.92]:41484) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1SXHrQ-0006jY-Jy for qemu-devel@nongnu.org; Wed, 23 May 2012 16:08:45 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1SXHrL-0000s9-Si for qemu-devel@nongnu.org; Wed, 23 May 2012 16:08:44 -0400 Received: from goliath.siemens.de ([192.35.17.28]:34641) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1SXHrL-0000r9-JZ for qemu-devel@nongnu.org; Wed, 23 May 2012 16:08:39 -0400 Message-ID: <4FBD43B5.1010008@siemens.com> Date: Wed, 23 May 2012 17:08:21 -0300 From: Jan Kiszka MIME-Version: 1.0 References: <1337791181-27446-1-git-send-email-imammedo@redhat.com> <1337791181-27446-5-git-send-email-imammedo@redhat.com> In-Reply-To: Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH qom-next 4/6] pc: move apic_mapped initialization into common apic init code List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Peter Maydell Cc: "aliguori@us.ibm.com" , "wei.liu2@citrix.com" , "ehabkost@redhat.com" , "stefano.stabellini@eu.citrix.com" , "sw@weilnetz.de" , "mtosatti@redhat.com" , "qemu-devel@nongnu.org" , "agraf@suse.de" , "blauwirbel@gmail.com" , "avi@redhat.com" , "pbonzini@redhat.com" , "anthony.perard@citrix.com" , Igor Mammedov , "afaerber@suse.de" On 2012-05-23 13:44, Peter Maydell wrote: > On 23 May 2012 17:39, Igor Mammedov wrote: >> @@ -295,6 +297,15 @@ static int apic_init_common(SysBusDevice *dev) >> >> sysbus_init_mmio(dev, &s->io_memory); >> >> + /* XXX: mapping more APICs at the same memory location */ >> + if (apic_mapped == 0) { >> + /* NOTE: the APIC is directly connected to the CPU - it is not >> + on the global memory bus. */ >> + /* XXX: what if the base changes? */ >> + sysbus_mmio_map(sysbus_from_qdev(&s->busdev.qdev), 0, MSI_ADDR_BASE); >> + apic_mapped = 1; >> + } >> + >> if (!vapic && s->vapic_control & VAPIC_ENABLE_MASK) { >> vapic = sysbus_create_simple("kvmvapic", -1, NULL); >> } > > This looks wrong -- sysbus device init functions shouldn't > be mapping MMIO regions themselves, in general. They should > expose MMIO regions to be mapped by whichever device or board > model creates them. Which is what the code before this patch > was doing -- why do you want to move this code? I fact, the CPU normally decides about where the APIC is mapped, specifically when it is remapped via the MSR (which QEMU cannot support yet). Well, unless we are talking about an external APIC and a 486 CPU. Then the board decides. But I guess we could live with ignoring that corner case and stick the mapping setup into the CPU initialization code. Jan -- Siemens AG, Corporate Technology, CT T DE IT 1 Corporate Competence Center Embedded Linux