From 3b3c60dc4cf8d2fa158accc2c401007439991224 Mon Sep 17 00:00:00 2001 From: JosJuice Date: Sat, 7 May 2022 14:21:30 +0200 Subject: [PATCH 1/3] PowerPC: Check page crossing for non-translated reads This fixes a problem where Dolphin could crash if a non-translated read crossed the end of a physical memory region. The same change was applied to WriteToHardware in ecbce0a. --- Source/Core/Core/PowerPC/MMU.cpp | 40 +++++++++++--------------------- 1 file changed, 14 insertions(+), 26 deletions(-) diff --git a/Source/Core/Core/PowerPC/MMU.cpp b/Source/Core/Core/PowerPC/MMU.cpp index 869a7a8648..aac5f35054 100644 --- a/Source/Core/Core/PowerPC/MMU.cpp +++ b/Source/Core/Core/PowerPC/MMU.cpp @@ -173,6 +173,20 @@ static void GenerateDSIException(u32 effective_address, bool write); template static T ReadFromHardware(u32 em_address) { + const u32 em_address_start_page = em_address & ~(HW_PAGE_SIZE - 1); + const u32 em_address_end_page = (em_address + sizeof(T) - 1) & ~(HW_PAGE_SIZE - 1); + if (em_address_start_page != em_address_end_page) + { + // This could be unaligned down to the byte level... hopefully this is rare, so doing it this + // way isn't too terrible. + // TODO: floats on non-word-aligned boundaries should technically cause alignment exceptions. + // Note that "word" means 32-bit, so paired singles or doubles might still be 32-bit aligned! + u64 var = 0; + for (u32 i = 0; i < sizeof(T); ++i) + var = (var << 8) | ReadFromHardware(em_address + i); + return static_cast(var); + } + if (!never_translate && MSR.DR) { auto translated_addr = TranslateAddress(em_address); @@ -182,35 +196,9 @@ static T ReadFromHardware(u32 em_address) GenerateDSIException(em_address, false); return 0; } - if ((em_address & (HW_PAGE_SIZE - 1)) > HW_PAGE_SIZE - sizeof(T)) - { - // This could be unaligned down to the byte level... hopefully this is rare, so doing it this - // way isn't too terrible. - // TODO: floats on non-word-aligned boundaries should technically cause alignment exceptions. - // Note that "word" means 32-bit, so paired singles or doubles might still be 32-bit aligned! - u32 em_address_next_page = (em_address + sizeof(T) - 1) & ~(HW_PAGE_SIZE - 1); - auto addr_next_page = TranslateAddress(em_address_next_page); - if (!addr_next_page.Success()) - { - if (flag == XCheckTLBFlag::Read) - GenerateDSIException(em_address_next_page, false); - return 0; - } - T var = 0; - u32 addr_translated = translated_addr.address; - for (u32 addr = em_address; addr < em_address + sizeof(T); addr++, addr_translated++) - { - if (addr == em_address_next_page) - addr_translated = addr_next_page.address; - var = (var << 8) | ReadFromHardware(addr_translated); - } - return var; - } em_address = translated_addr.address; } - // TODO: Make sure these are safe for unaligned addresses. - if (Memory::m_pRAM && (em_address & 0xF8000000) == 0x00000000) { // Handle RAM; the masking intentionally discards bits (essentially creating From ed40b439605d0fc509f61f89271a70bc233ad96b Mon Sep 17 00:00:00 2001 From: JosJuice Date: Sat, 7 May 2022 14:32:45 +0200 Subject: [PATCH 2/3] PowerPC: Reorder code in ReadFromHardware This refactorization is done just to match the order that I made WriteToHardware use in 543ed8a. For WriteToHardware, it's important that things like MMIO and gather pipe are handled before we reach a special piece of code that only should get triggered for writes that hit memory directly, but for ReadFromHardware we don't have any code like that. --- Source/Core/Core/PowerPC/MMU.cpp | 34 ++++++++++++++++---------------- 1 file changed, 17 insertions(+), 17 deletions(-) diff --git a/Source/Core/Core/PowerPC/MMU.cpp b/Source/Core/Core/PowerPC/MMU.cpp index aac5f35054..ab460f1674 100644 --- a/Source/Core/Core/PowerPC/MMU.cpp +++ b/Source/Core/Core/PowerPC/MMU.cpp @@ -199,11 +199,27 @@ static T ReadFromHardware(u32 em_address) em_address = translated_addr.address; } + if (flag == XCheckTLBFlag::Read && (em_address & 0xF8000000) == 0x08000000) + { + if (em_address < 0x0c000000) + return EFB_Read(em_address); + else + return static_cast(Memory::mmio_mapping->Read>(em_address)); + } + + // Locked L1 technically doesn't have a fixed address, but games all use 0xE0000000. + if (Memory::m_pL1Cache && (em_address >> 28) == 0xE && + (em_address < (0xE0000000 + Memory::GetL1CacheSize()))) + { + T value; + std::memcpy(&value, &Memory::m_pL1Cache[em_address & 0x0FFFFFFF], sizeof(T)); + return bswap(value); + } + if (Memory::m_pRAM && (em_address & 0xF8000000) == 0x00000000) { // Handle RAM; the masking intentionally discards bits (essentially creating // mirrors of memory). - // TODO: Only the first GetRamSizeReal() is supposed to be backed by actual memory. T value; std::memcpy(&value, &Memory::m_pRAM[em_address & Memory::GetRamMask()], sizeof(T)); return bswap(value); @@ -217,14 +233,6 @@ static T ReadFromHardware(u32 em_address) return bswap(value); } - // Locked L1 technically doesn't have a fixed address, but games all use 0xE0000000. - if (Memory::m_pL1Cache && (em_address >> 28) == 0xE && - (em_address < (0xE0000000 + Memory::GetL1CacheSize()))) - { - T value; - std::memcpy(&value, &Memory::m_pL1Cache[em_address & 0x0FFFFFFF], sizeof(T)); - return bswap(value); - } // In Fake-VMEM mode, we need to map the memory somewhere into // physical memory for BAT translation to work; we currently use // [0x7E000000, 0x80000000). @@ -235,14 +243,6 @@ static T ReadFromHardware(u32 em_address) return bswap(value); } - if (flag == XCheckTLBFlag::Read && (em_address & 0xF8000000) == 0x08000000) - { - if (em_address < 0x0c000000) - return EFB_Read(em_address); - else - return (T)Memory::mmio_mapping->Read::type>(em_address); - } - PanicAlertFmt("Unable to resolve read address {:x} PC {:x}", em_address, PC); return 0; } From b6b70304829c16fad7b2e48c42d0110d732404dd Mon Sep 17 00:00:00 2001 From: JosJuice Date: Sat, 7 May 2022 19:37:44 +0200 Subject: [PATCH 3/3] PowerPC: Add HW_PAGE_MASK constant --- Source/Core/Core/PowerPC/MMU.cpp | 8 ++++---- Source/Core/Core/PowerPC/MMU.h | 1 + 2 files changed, 5 insertions(+), 4 deletions(-) diff --git a/Source/Core/Core/PowerPC/MMU.cpp b/Source/Core/Core/PowerPC/MMU.cpp index ab460f1674..6c0867deb6 100644 --- a/Source/Core/Core/PowerPC/MMU.cpp +++ b/Source/Core/Core/PowerPC/MMU.cpp @@ -173,8 +173,8 @@ static void GenerateDSIException(u32 effective_address, bool write); template static T ReadFromHardware(u32 em_address) { - const u32 em_address_start_page = em_address & ~(HW_PAGE_SIZE - 1); - const u32 em_address_end_page = (em_address + sizeof(T) - 1) & ~(HW_PAGE_SIZE - 1); + const u32 em_address_start_page = em_address & ~HW_PAGE_MASK; + const u32 em_address_end_page = (em_address + sizeof(T) - 1) & ~HW_PAGE_MASK; if (em_address_start_page != em_address_end_page) { // This could be unaligned down to the byte level... hopefully this is rare, so doing it this @@ -252,8 +252,8 @@ static void WriteToHardware(u32 em_address, const u32 data, const u32 size) { DEBUG_ASSERT(size <= 4); - const u32 em_address_start_page = em_address & ~(HW_PAGE_SIZE - 1); - const u32 em_address_end_page = (em_address + size - 1) & ~(HW_PAGE_SIZE - 1); + const u32 em_address_start_page = em_address & ~HW_PAGE_MASK; + const u32 em_address_end_page = (em_address + size - 1) & ~HW_PAGE_MASK; if (em_address_start_page != em_address_end_page) { // The write crosses a page boundary. Break it up into two writes. diff --git a/Source/Core/Core/PowerPC/MMU.h b/Source/Core/Core/PowerPC/MMU.h index afe36a9875..726e861f62 100644 --- a/Source/Core/Core/PowerPC/MMU.h +++ b/Source/Core/Core/PowerPC/MMU.h @@ -215,6 +215,7 @@ inline bool TranslateBatAddess(const BatTable& bat_table, u32* address, bool* wi } constexpr size_t HW_PAGE_SIZE = 4096; +constexpr size_t HW_PAGE_MASK = HW_PAGE_SIZE - 1; constexpr u32 HW_PAGE_INDEX_SHIFT = 12; constexpr u32 HW_PAGE_INDEX_MASK = 0x3f;