From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([208.118.235.92]:47700) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1UWkoG-0002RK-Em for qemu-devel@nongnu.org; Mon, 29 Apr 2013 05:55:49 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1UWkoC-0000NK-NQ for qemu-devel@nongnu.org; Mon, 29 Apr 2013 05:55:48 -0400 Received: from mx1.redhat.com ([209.132.183.28]:30916) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1UWkoC-0000NE-FT for qemu-devel@nongnu.org; Mon, 29 Apr 2013 05:55:44 -0400 Date: Mon, 29 Apr 2013 11:55:06 +0200 From: Igor Mammedov Message-ID: <20130429115506.0b5e8d15@thinkpad> In-Reply-To: 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> <517BC0A0.2080700@redhat.com> Mime-Version: 1.0 Content-Type: text/plain; charset=US-ASCII 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, yang.z.zhang@intel.com, alex.williamson@redhat.com, kraxel@redhat.com, anthony.perard@citrix.com, Paolo Bonzini , lcapitulino@redhat.com, afaerber@suse.de, stefano.stabellini@eu.citrix.com, rth@twiddle.net On Sat, 27 Apr 2013 20:57:26 +0000 Blue Swirl wrote: > On Sat, Apr 27, 2013 at 12:12 PM, Paolo Bonzini wrote: > > 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. > > These questions ask if there is a risk of benevolent guests performing > these activities and I agree that the chances are close to zero. > > But the interesting question is to ask if a malevolent guest can bring > down a VM uncontrollably this way and I think it only needs a few > elevated privileges in a guest to do this. > > The fix is to avoid abort(), which is a separate issue to whether the > address base should be resolved for each KVM ioctl or not. > > > > > 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. > > At least the guest needs to be stopped. Perhaps we should have a > common function which does this and logs the guest error so we can > start replacing calls to abort() with it. > > > > > Paolo > It looks like discussion got deviated from what patch does to another issue. this patch doesn't address/change the way how/when base_address should be set/updated but it has it's benefits as well: - removes/cleanups access to private field of parent, which allows to convert it to non SysBusDevice - extended memory_region_find() opens venue for cleaning-up/re-factoring devices that use framebuffer which are forced currently to access system_address_space directly. -- Regards, Igor