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(-) > >
