From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([208.118.235.92]:42368) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1UW3zT-000369-Iy for qemu-devel@nongnu.org; Sat, 27 Apr 2013 08:12:32 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1UW3zS-00077a-Cz for qemu-devel@nongnu.org; Sat, 27 Apr 2013 08:12:31 -0400 Received: from mail-bk0-x22a.google.com ([2a00:1450:4008:c01::22a]:38698) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1UW3zS-000752-6D for qemu-devel@nongnu.org; Sat, 27 Apr 2013 08:12:30 -0400 Received: by mail-bk0-f42.google.com with SMTP id jf3so252527bkc.15 for ; Sat, 27 Apr 2013 05:12:29 -0700 (PDT) Sender: Paolo Bonzini Message-ID: <517BC0A0.2080700@redhat.com> Date: Sat, 27 Apr 2013 14:12:16 +0200 From: Paolo Bonzini MIME-Version: 1.0 References: <1366705795-24732-1-git-send-email-imammedo@redhat.com> <1366705795-24732-18-git-send-email-imammedo@redhat.com> <20130426161754.583e3f6b@thinkpad> <20130426194616.50534c82@thinkpad> <517AFBF8.5090100@redhat.com> In-Reply-To: Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH 17/21] introduce memory_region_get_address() and use it in kvm/ioapic List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Blue Swirl Cc: kwolf@redhat.com, peter.maydell@linaro.org, aliguori@us.ibm.com, ehabkost@redhat.com, gleb@redhat.com, mst@redhat.com, jan.kiszka@siemens.com, quintela@redhat.com, claudio.fontana@huawei.com, armbru@redhat.com, aderumier@odiso.com, qemu-devel@nongnu.org, anthony.perard@citrix.com, alex.williamson@redhat.com, kraxel@redhat.com, yang.z.zhang@intel.com, Igor Mammedov , lcapitulino@redhat.com, afaerber@suse.de, stefano.stabellini@eu.citrix.com, rth@twiddle.net Il 27/04/2013 12:09, Blue Swirl ha scritto: > On Fri, Apr 26, 2013 at 10:13 PM, Paolo Bonzini wrote: >> Il 26/04/2013 19:46, Igor Mammedov ha scritto: >>>>> But as the address can't be changed (yet), the entire patch could be simply: >>>>> - kioapic->base_address = s->busdev.mmio[0].addr; >>>>> + kioapic->base_address = IO_APIC_DEFAULT_ADDRESS; >>> It's a bit fragile, but that for sure simpler and can work. >>> >>> Jan, Paolo, >>> Are you ok with this approach? >>> >> >> I think extending memory_region_find is a good idea anyway, and at this >> point I don't see a reason to do the above change... > > The reasoning was in the part that Igor cut off: > > "Later, when it's possible to change the address via PIIX3 registers, > we can adjust the base and pass that properly to kioapic and on to > KVM. > > Resolving the base address every time when kvm_ioapic_put() is called > is also less efficient, assuming of course that the base address > changes less often than the KVM ioctl is used." > > I think the patch is a bit flawed. If the guest maps something else on > top of IOAPIC, like LAPIC (which should be in CPU specific address > spaces, but for now it lives in the global system memory space), the > guest could trigger the abort() by resetting the system. The questions are, in order of importance: (1) what privileges would this require in the guest? Answer: a lot. (2) is this likely to happen by chance? Answer: no, not at all. (3) is there a workaround? Answer: yes, disable in-kernel irqchip. Simply setting IO_APIC_DEFAULT_ADDRESS is also flawed in my opinion. I'm not sure the in-kernel irqchip handles correctly an overlap between the IOAPIC and LAPIC regions, maybe an abort is predictable after all. Paolo