From 83f6e9ea725be6d1bed821c4797f8e464ff9c32a Mon Sep 17 00:00:00 2001 From: Lioncash Date: Wed, 21 Nov 2018 18:22:24 -0500 Subject: [PATCH 1/5] kernel/handle_table: Default destructor in the cpp file We don't need to potentially inline the teardown logic of all of the handle instances. --- src/core/hle/kernel/handle_table.cpp | 2 ++ src/core/hle/kernel/handle_table.h | 1 + 2 files changed, 3 insertions(+) diff --git a/src/core/hle/kernel/handle_table.cpp b/src/core/hle/kernel/handle_table.cpp index 15dcc4583..7be2dcb12 100644 --- a/src/core/hle/kernel/handle_table.cpp +++ b/src/core/hle/kernel/handle_table.cpp @@ -17,6 +17,8 @@ HandleTable::HandleTable(KernelSystem& kernel) : kernel(kernel) { Clear(); } +HandleTable::~HandleTable() = default; + ResultVal HandleTable::Create(SharedPtr obj) { DEBUG_ASSERT(obj != nullptr); diff --git a/src/core/hle/kernel/handle_table.h b/src/core/hle/kernel/handle_table.h index 00cb47a33..7f76f53d6 100644 --- a/src/core/hle/kernel/handle_table.h +++ b/src/core/hle/kernel/handle_table.h @@ -43,6 +43,7 @@ enum KernelHandle : Handle { class HandleTable final : NonCopyable { public: explicit HandleTable(KernelSystem& kernel); + ~HandleTable(); /** * Allocates a handle for the given object. From 662c3ff684cf118605ad771e0d2c9200b3574b01 Mon Sep 17 00:00:00 2001 From: Lioncash Date: Wed, 21 Nov 2018 18:30:58 -0500 Subject: [PATCH 2/5] kernel/handle_table: Move private static functions into the cpp file These don't depend on class state, and are effectively implementation details, so they can go into the cpp file . --- src/core/hle/kernel/handle_table.cpp | 9 +++++++++ src/core/hle/kernel/handle_table.h | 7 ------- 2 files changed, 9 insertions(+), 7 deletions(-) diff --git a/src/core/hle/kernel/handle_table.cpp b/src/core/hle/kernel/handle_table.cpp index 7be2dcb12..2784ca8b0 100644 --- a/src/core/hle/kernel/handle_table.cpp +++ b/src/core/hle/kernel/handle_table.cpp @@ -11,6 +11,15 @@ #include "core/hle/kernel/thread.h" namespace Kernel { +namespace { +constexpr u16 GetSlot(Handle handle) { + return handle >> 15; +} + +constexpr u16 GetGeneration(Handle handle) { + return handle & 0x7FFF; +} +} // Anonymous namespace HandleTable::HandleTable(KernelSystem& kernel) : kernel(kernel) { next_generation = 1; diff --git a/src/core/hle/kernel/handle_table.h b/src/core/hle/kernel/handle_table.h index 7f76f53d6..59588d225 100644 --- a/src/core/hle/kernel/handle_table.h +++ b/src/core/hle/kernel/handle_table.h @@ -96,13 +96,6 @@ private: */ static const std::size_t MAX_COUNT = 4096; - static u16 GetSlot(Handle handle) { - return handle >> 15; - } - static u16 GetGeneration(Handle handle) { - return handle & 0x7FFF; - } - /// Stores the Object referenced by the handle or null if the slot is empty. std::array, MAX_COUNT> objects; From 1cb9bea504cf6804f982467a00c37561e98c4dfd Mon Sep 17 00:00:00 2001 From: Lioncash Date: Mon, 19 Nov 2018 08:29:25 -0500 Subject: [PATCH 3/5] kernel/shared_memory: Make data members private Rather than allow unfettered access to the class internals, we hide all members by default and create and API that other code can operate against. --- src/core/hle/applets/swkbd.cpp | 2 +- src/core/hle/kernel/shared_memory.h | 37 +++++++++++++++++++--------- src/core/hle/service/apt/apt.cpp | 2 +- src/core/hle/service/http_c.cpp | 2 +- src/core/hle/service/ir/ir_user.cpp | 2 +- src/core/hle/service/mic_u.cpp | 2 +- src/core/hle/service/nwm/nwm_uds.cpp | 2 +- 7 files changed, 31 insertions(+), 18 deletions(-) diff --git a/src/core/hle/applets/swkbd.cpp b/src/core/hle/applets/swkbd.cpp index 0de0555c0..fc185bd7b 100644 --- a/src/core/hle/applets/swkbd.cpp +++ b/src/core/hle/applets/swkbd.cpp @@ -65,7 +65,7 @@ ResultCode SoftwareKeyboard::StartImpl(Service::APT::AppletStartupParameter cons boost::static_pointer_cast(parameter.object); // TODO(Subv): Verify if this is the correct behavior - memset(text_memory->GetPointer(), 0, text_memory->size); + memset(text_memory->GetPointer(), 0, text_memory->GetSize()); DrawScreenKeyboard(); diff --git a/src/core/hle/kernel/shared_memory.h b/src/core/hle/kernel/shared_memory.h index a48f607ba..7ba45dd00 100644 --- a/src/core/hle/kernel/shared_memory.h +++ b/src/core/hle/kernel/shared_memory.h @@ -20,12 +20,25 @@ public: std::string GetName() const override { return name; } + void SetName(std::string name) { + this->name = name; + } static const HandleType HANDLE_TYPE = HandleType::SharedMemory; HandleType GetHandleType() const override { return HANDLE_TYPE; } + /// Gets the size of the underlying memory block in bytes. + u64 GetSize() const { + return size; + } + + /// Gets the linear heap physical offset + u64 GetLinearHeapPhysicalOffset() const { + return linear_heap_phys_offset; + } + /** * Converts the specified MemoryPermission into the equivalent VMAPermission. * @param permission The MemoryPermission to convert. @@ -57,30 +70,30 @@ public: */ u8* GetPointer(u32 offset = 0); - /// Process that created this shared memory block. - Process* owner_process; - /// Address of shared memory block in the owner process if specified. - VAddr base_address; +private: + explicit SharedMemory(KernelSystem& kernel); + ~SharedMemory() override; + /// Offset in FCRAM of the shared memory block in the linear heap if no address was specified /// during creation. - PAddr linear_heap_phys_offset; + PAddr linear_heap_phys_offset = 0; /// Backing memory for this shared memory block. std::vector> backing_blocks; /// Size of the memory block. Page-aligned. - u32 size; + u32 size = 0; /// Permission restrictions applied to the process which created the block. - MemoryPermission permissions; + MemoryPermission permissions{}; /// Permission restrictions applied to other processes mapping the block. - MemoryPermission other_permissions; + MemoryPermission other_permissions{}; + /// Process that created this shared memory block. + SharedPtr owner_process; + /// Address of shared memory block in the owner process if specified. + VAddr base_address = 0; /// Name of shared memory object. std::string name; MemoryRegionInfo::IntervalSet holding_memory; -private: - explicit SharedMemory(KernelSystem& kernel); - ~SharedMemory() override; - friend class KernelSystem; KernelSystem& kernel; }; diff --git a/src/core/hle/service/apt/apt.cpp b/src/core/hle/service/apt/apt.cpp index db8247a2a..6b779bc30 100644 --- a/src/core/hle/service/apt/apt.cpp +++ b/src/core/hle/service/apt/apt.cpp @@ -217,7 +217,7 @@ void Module::Interface::GetSharedFont(Kernel::HLERequestContext& ctx) { // shared font, different linear heap region would have required shared font to relocate // according to two different addresses at the same time, which is impossible. VAddr target_address = - apt->shared_font_mem->linear_heap_phys_offset + Memory::LINEAR_HEAP_VADDR; + apt->shared_font_mem->GetLinearHeapPhysicalOffset() + Memory::LINEAR_HEAP_VADDR; if (!apt->shared_font_relocated) { BCFNT::RelocateSharedFont(apt->shared_font_mem, target_address); apt->shared_font_relocated = true; diff --git a/src/core/hle/service/http_c.cpp b/src/core/hle/service/http_c.cpp index f55d6ed0e..e873e914e 100644 --- a/src/core/hle/service/http_c.cpp +++ b/src/core/hle/service/http_c.cpp @@ -50,7 +50,7 @@ void HTTP_C::Initialize(Kernel::HLERequestContext& ctx) { u32 pid = rp.PopPID(); shared_memory = rp.PopObject(); if (shared_memory) { - shared_memory->name = "HTTP_C:shared_memory"; + shared_memory->SetName("HTTP_C:shared_memory"); } LOG_WARNING(Service_HTTP, "(STUBBED) called, shared memory size: {} pid: {}", shmem_size, pid); diff --git a/src/core/hle/service/ir/ir_user.cpp b/src/core/hle/service/ir/ir_user.cpp index e0158f67e..c4466a13f 100644 --- a/src/core/hle/service/ir/ir_user.cpp +++ b/src/core/hle/service/ir/ir_user.cpp @@ -240,7 +240,7 @@ void IR_USER::InitializeIrNopShared(Kernel::HLERequestContext& ctx) { IPC::RequestBuilder rb = rp.MakeBuilder(1, 0); - shared_memory->name = "IR_USER: shared memory"; + shared_memory->SetName("IR_USER: shared memory"); receive_buffer = std::make_unique(shared_memory, 0x10, 0x20, recv_buff_packet_count, recv_buff_size); diff --git a/src/core/hle/service/mic_u.cpp b/src/core/hle/service/mic_u.cpp index ce9594751..6c97282aa 100644 --- a/src/core/hle/service/mic_u.cpp +++ b/src/core/hle/service/mic_u.cpp @@ -40,7 +40,7 @@ struct MIC_U::Impl { shared_memory = rp.PopObject(); if (shared_memory) { - shared_memory->name = "MIC_U:shared_memory"; + shared_memory->SetName("MIC_U:shared_memory"); } IPC::RequestBuilder rb = rp.MakeBuilder(1, 0); diff --git a/src/core/hle/service/nwm/nwm_uds.cpp b/src/core/hle/service/nwm/nwm_uds.cpp index 85b309f43..755da4f1d 100644 --- a/src/core/hle/service/nwm/nwm_uds.cpp +++ b/src/core/hle/service/nwm/nwm_uds.cpp @@ -724,7 +724,7 @@ void NWM_UDS::InitializeWithVersion(Kernel::HLERequestContext& ctx) { initialized = true; - ASSERT_MSG(recv_buffer_memory->size == sharedmem_size, "Invalid shared memory size."); + ASSERT_MSG(recv_buffer_memory->GetSize() == sharedmem_size, "Invalid shared memory size."); if (auto room_member = Network::GetRoomMember().lock()) { wifi_packet_received = room_member->BindOnWifiPacketReceived(OnWifiPacketReceived); From 0f544af89a63a9aac6ddc3d57e093fa6c91fcb49 Mon Sep 17 00:00:00 2001 From: Lioncash Date: Mon, 19 Nov 2018 09:00:32 -0500 Subject: [PATCH 4/5] kernel/shared_memory: Add a const qualified member function overload for GetPointer() Given this doesn't mutate instance state, we can provide a const-qualified variant as well. --- src/core/hle/kernel/shared_memory.cpp | 7 +++++++ src/core/hle/kernel/shared_memory.h | 9 ++++++++- 2 files changed, 15 insertions(+), 1 deletion(-) diff --git a/src/core/hle/kernel/shared_memory.cpp b/src/core/hle/kernel/shared_memory.cpp index eec74bfde..92d6985c3 100644 --- a/src/core/hle/kernel/shared_memory.cpp +++ b/src/core/hle/kernel/shared_memory.cpp @@ -196,4 +196,11 @@ u8* SharedMemory::GetPointer(u32 offset) { return backing_blocks[0].first + offset; } +const u8* SharedMemory::GetPointer(u32 offset) const { + if (backing_blocks.size() != 1) { + LOG_WARNING(Kernel, "Unsafe GetPointer on discontinuous SharedMemory"); + } + return backing_blocks[0].first + offset; +} + } // namespace Kernel diff --git a/src/core/hle/kernel/shared_memory.h b/src/core/hle/kernel/shared_memory.h index 7ba45dd00..510d47631 100644 --- a/src/core/hle/kernel/shared_memory.h +++ b/src/core/hle/kernel/shared_memory.h @@ -66,10 +66,17 @@ public: /** * Gets a pointer to the shared memory block * @param offset Offset from the start of the shared memory block to get pointer - * @return Pointer to the shared memory block from the specified offset + * @return A pointer to the shared memory block from the specified offset */ u8* GetPointer(u32 offset = 0); + /** + * Gets a constant pointer to the shared memory block + * @param offset Offset from the start of the shared memory block to get pointer + * @return A constant pointer to the shared memory block from the specified offset + */ + const u8* GetPointer(u32 offset = 0) const; + private: explicit SharedMemory(KernelSystem& kernel); ~SharedMemory() override; From 86c36cb17652d8ee880cd5c6e10ecaf6a0320337 Mon Sep 17 00:00:00 2001 From: Lioncash Date: Mon, 19 Nov 2018 09:05:04 -0500 Subject: [PATCH 5/5] kernel/shared_memory: Make Map() and Unmap() take the target process by reference rather than as a pointer Both member functions assume the passed in target process will not be null. Instead of making this assumption implicit, we can change the functions to be references and enforce this at the type-system level. --- src/core/hle/kernel/shared_memory.cpp | 16 ++++++++-------- src/core/hle/kernel/shared_memory.h | 6 +++--- src/core/hle/kernel/svc.cpp | 4 ++-- 3 files changed, 13 insertions(+), 13 deletions(-) diff --git a/src/core/hle/kernel/shared_memory.cpp b/src/core/hle/kernel/shared_memory.cpp index 92d6985c3..f1acca74d 100644 --- a/src/core/hle/kernel/shared_memory.cpp +++ b/src/core/hle/kernel/shared_memory.cpp @@ -95,11 +95,11 @@ SharedPtr KernelSystem::CreateSharedMemoryForApplet( return shared_memory; } -ResultCode SharedMemory::Map(Process* target_process, VAddr address, MemoryPermission permissions, +ResultCode SharedMemory::Map(Process& target_process, VAddr address, MemoryPermission permissions, MemoryPermission other_permissions) { MemoryPermission own_other_permissions = - target_process == owner_process ? this->permissions : this->other_permissions; + &target_process == owner_process ? this->permissions : this->other_permissions; // Automatically allocated memory blocks can only be mapped with other_permissions = DontCare if (base_address == 0 && other_permissions != MemoryPermission::DontCare) { @@ -155,7 +155,7 @@ ResultCode SharedMemory::Map(Process* target_process, VAddr address, MemoryPermi target_address = linear_heap_phys_offset + Memory::LINEAR_HEAP_VADDR; } - auto vma = target_process->vm_manager.FindVMA(target_address); + auto vma = target_process.vm_manager.FindVMA(target_address); if (vma->second.type != VMAType::Free || vma->second.base + vma->second.size < target_address + size) { LOG_ERROR(Kernel, @@ -167,20 +167,20 @@ ResultCode SharedMemory::Map(Process* target_process, VAddr address, MemoryPermi // Map the memory block into the target process VAddr interval_target = target_address; for (const auto& interval : backing_blocks) { - auto vma = target_process->vm_manager.MapBackingMemory( - interval_target, interval.first, interval.second, MemoryState::Shared); + auto vma = target_process.vm_manager.MapBackingMemory(interval_target, interval.first, + interval.second, MemoryState::Shared); ASSERT(vma.Succeeded()); - target_process->vm_manager.Reprotect(vma.Unwrap(), ConvertPermissions(permissions)); + target_process.vm_manager.Reprotect(vma.Unwrap(), ConvertPermissions(permissions)); interval_target += interval.second; } return RESULT_SUCCESS; } -ResultCode SharedMemory::Unmap(Process* target_process, VAddr address) { +ResultCode SharedMemory::Unmap(Process& target_process, VAddr address) { // TODO(Subv): Verify what happens if the application tries to unmap an address that is not // mapped to a SharedMemory. - return target_process->vm_manager.UnmapRange(address, size); + return target_process.vm_manager.UnmapRange(address, size); } VMAPermission SharedMemory::ConvertPermissions(MemoryPermission permission) { diff --git a/src/core/hle/kernel/shared_memory.h b/src/core/hle/kernel/shared_memory.h index 510d47631..b8760ac4b 100644 --- a/src/core/hle/kernel/shared_memory.h +++ b/src/core/hle/kernel/shared_memory.h @@ -52,16 +52,16 @@ public: * @param permissions Memory block map permissions (specified by SVC field) * @param other_permissions Memory block map other permissions (specified by SVC field) */ - ResultCode Map(Process* target_process, VAddr address, MemoryPermission permissions, + ResultCode Map(Process& target_process, VAddr address, MemoryPermission permissions, MemoryPermission other_permissions); /** * Unmaps a shared memory block from the specified address in system memory - * @param target_process Process from which to umap the memory block. + * @param target_process Process from which to unmap the memory block. * @param address Address in system memory where the shared memory block is mapped * @return Result code of the unmap operation */ - ResultCode Unmap(Process* target_process, VAddr address); + ResultCode Unmap(Process& target_process, VAddr address); /** * Gets a pointer to the shared memory block diff --git a/src/core/hle/kernel/svc.cpp b/src/core/hle/kernel/svc.cpp index a896f770f..ce69b2d6a 100644 --- a/src/core/hle/kernel/svc.cpp +++ b/src/core/hle/kernel/svc.cpp @@ -322,7 +322,7 @@ ResultCode SVC::MapMemoryBlock(Handle handle, u32 addr, u32 permissions, u32 oth case MemoryPermission::WriteExecute: case MemoryPermission::ReadWriteExecute: case MemoryPermission::DontCare: - return shared_memory->Map(kernel.GetCurrentProcess().get(), addr, permissions_type, + return shared_memory->Map(*kernel.GetCurrentProcess(), addr, permissions_type, static_cast(other_permissions)); default: LOG_ERROR(Kernel_SVC, "unknown permissions=0x{:08X}", permissions); @@ -341,7 +341,7 @@ ResultCode SVC::UnmapMemoryBlock(Handle handle, u32 addr) { if (shared_memory == nullptr) return ERR_INVALID_HANDLE; - return shared_memory->Unmap(current_process.get(), addr); + return shared_memory->Unmap(*current_process, addr); } /// Connect to an OS service given the port name, returns the handle to the port to out