From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:49020) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1VeR6g-0005LE-Iv for qemu-devel@nongnu.org; Thu, 07 Nov 2013 10:02:55 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1VeR6b-0000GV-IS for qemu-devel@nongnu.org; Thu, 07 Nov 2013 10:02:50 -0500 Received: from mx1.redhat.com ([209.132.183.28]:4783) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1VeR6b-0000GI-Bb for qemu-devel@nongnu.org; Thu, 07 Nov 2013 10:02:45 -0500 Date: Thu, 7 Nov 2013 17:05:01 +0200 From: "Michael S. Tsirkin" Message-ID: <20131107150501.GA3976@redhat.com> References: <1383820884-29596-1-git-send-email-marcel.a@redhat.com> <527B9EE3.3030507@redhat.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <527B9EE3.3030507@redhat.com> Subject: Re: [Qemu-devel] [PATCH for-1.7 v2 0/8] fix address space size issue List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Paolo Bonzini Cc: peter.maydell@linaro.org, ehabkost@redhat.com, Marcel Apfelbaum , qemu-devel@nongnu.org, jan.kiszka@siemens.com, agraf@suse.de, lcapitulino@redhat.com, aliguori@amazon.com, afaerber@suse.de On Thu, Nov 07, 2013 at 03:08:35PM +0100, Paolo Bonzini wrote: > Il 07/11/2013 11:41, Marcel Apfelbaum ha scritto: > > A bug reported by Luiz Capitulino let us to find > > several bugs in memory address space setup. > > > > One issue is that gdb stub can give us arbitrary addresses > > and we'll try to access them. > > Since our lookup ignored high bits in the address, > > we hit a wrong section and got a crash. > > In fact, PCI devices can access arbitrary addresses too, > > so we should just make lookup robust against this case. > > > > Another issue has to do with size of regions. > > memory API uses UINT64_MAX so say "all 64 bit" but > > some devices mistakenly used INT64_MAX. > > > > It should not affect most systems in practice as > > everything should be limited by address space size, > > but it's an API misuse that we should not keep around, > > and it will become a problem if a system with 64 bit > > target address hits this path. > > > > Patch 1 introduces TARGET_PHYS_ADDR_SPACE_MAX that is > > the max size for memory regions rendered by exec. > > Patches 2-3 limits the size of memory regions used by exec.c. > > Patch 4 fixes an actual bug. > > The rest of patches make code cleaner and more robust. > > I think this is wrong. > > There is no reason why boards should be using TARGET_PHYS_ADDR_SPACE_MAX > if the PCI address space is 64-bit. Many files in hw/ do not even have > access to TARGET_PHYS_ADDR_SPACE_MAX, which is only available to files > that are compiled per-target. > > The fact that you have to reduce the IOMMU address spaces from 64 bits > (UINT64_MAX, not the buggy INT64_MAX) to something else is a red flag. > If the IOMMU supports 64-bit addressing, the IOMMU regions should have > 2^64 bits as their length. Yes, it works by chance now, but it _does_ > work for 2^64-bit size regions: you can do DMA to a high address (say > 0xf << 60), the IOMMU tables will convert to a low address that fits > within TARGET_PHYS_ADDR_SPACE_BITS, and the everything will be fine. > Patches 4 and 6 change that. > > So, ack for patch 5-7-8, which should also be enough to fix the problem > that Luiz reported. Not at all. As long as exec.c ignores high bits, any access there will end up in the wrong region. We might not get a crash but we'll get guest memory corruption. Not good. > For everything else the solution is to make memory > dispatch use the whole 64 bits, as in > http://permalink.gmane.org/gmane.comp.emulators.qemu/240229. If you > want to delay that to 1.8 that's fine. > > Paolo > > > > > Marcel Apfelbaum (3): > > exec: declare TARGET_PHYS_ADDR_SPACE_MAX to limit memory regions > > rendered by exec > > hw/alpha: limit iommu-typhoon memory size > > hw/ppc: limit iommu-spapr memory size > > > > Michael S. Tsirkin (4): > > exec: don't ignore high address bits on lookup > > pci: fix address space size for bridge > > exec: don't ignore high address bits on set > > spapr_pci: s/INT64_MAX/UINT64_MAX/ > > > > Paolo Bonzini (1): > > pc: s/INT64_MAX/UINT64_MAX/ > > > > exec.c | 18 ++++++++++++++++++ > > hw/alpha/typhoon.c | 2 +- > > hw/i386/pc_piix.c | 2 +- > > hw/i386/pc_q35.c | 2 +- > > hw/pci/pci_bridge.c | 2 +- > > hw/ppc/spapr_iommu.c | 2 +- > > hw/ppc/spapr_pci.c | 2 +- > > include/exec/address-spaces.h | 4 ++++ > > 8 files changed, 28 insertions(+), 6 deletions(-) > >