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