From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:36742) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1VeQGO-0004Sn-Gb for qemu-devel@nongnu.org; Thu, 07 Nov 2013 09:08:53 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1VeQGJ-0007gL-Ef for qemu-devel@nongnu.org; Thu, 07 Nov 2013 09:08:48 -0500 Received: from mx1.redhat.com ([209.132.183.28]:8342) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1VeQGJ-0007gE-6L for qemu-devel@nongnu.org; Thu, 07 Nov 2013 09:08:43 -0500 Message-ID: <527B9EE3.3030507@redhat.com> Date: Thu, 07 Nov 2013 15:08:35 +0100 From: Paolo Bonzini MIME-Version: 1.0 References: <1383820884-29596-1-git-send-email-marcel.a@redhat.com> In-Reply-To: <1383820884-29596-1-git-send-email-marcel.a@redhat.com> Content-Type: text/plain; charset=ISO-8859-15 Content-Transfer-Encoding: 7bit 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: Marcel Apfelbaum Cc: peter.maydell@linaro.org, ehabkost@redhat.com, mst@redhat.com, qemu-devel@nongnu.org, jan.kiszka@siemens.com, agraf@suse.de, lcapitulino@redhat.com, aliguori@amazon.com, afaerber@suse.de 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. 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(-) >