-
-
Notifications
You must be signed in to change notification settings - Fork 72
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[WIP] Memory management improvements #508
base: master
Are you sure you want to change the base?
Conversation
Disables a lot of functionality... but I didn't want to commit too much to this commit Also reworks virtual memory management somewhat (but needs more work)
Previously all non-free blocks were marked as Reserved
Can now use a single function to change either state, permissions, or both Also merge vmem blocks that have the same state and permissions
Fix minor bug with permission tracking
Also do a sanity check to make sure the memory region is free for linear allocations
Also move TLS to Base region
Not quite correct, but nothing to be done until process management is improved Also remove the stack limit for CXIs (thanks amogus)
Not really correct, but it should be accurate for applications at least
@@ -265,7 +266,8 @@ void Kernel::getProcessInfo() { | |||
// According to 3DBrew: Amount of private (code, data, heap) memory used by the process + total supervisor-mode | |||
// stack size + page-rounded size of the external handle table | |||
case 2: | |||
regs[1] = mem.getUsedUserMem(); | |||
// FIXME | |||
regs[1] = fcramManager.getUsedCount(FcramRegion::App); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
TODO for future me before merging: Check if this needs * pageSize
@@ -342,7 +344,7 @@ void Kernel::getSystemInfo() { | |||
switch (subtype) { | |||
// Total used memory size in the APPLICATION memory region | |||
case 1: | |||
regs[1] = mem.getUsedUserMem(); | |||
regs[1] = fcramManager.getUsedCount(FcramRegion::App); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ditto
u8* fcram; | ||
u8* dspRam; // Provided to us by Audio | ||
u8* vram; // Provided to the memory class by the GPU class | ||
|
||
u64& cpuTicks; // Reference to the CPU tick counter | ||
using SharedMemoryBlock = KernelMemoryTypes::SharedMemoryBlock; | ||
|
||
// TODO: remove this reference when Peach's excellent page table code is moved to a better home |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
:(
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's true
@@ -175,6 +175,7 @@ set(KERNEL_SOURCE_FILES src/core/kernel/kernel.cpp src/core/kernel/resource_limi | |||
src/core/kernel/address_arbiter.cpp src/core/kernel/error.cpp | |||
src/core/kernel/file_operations.cpp src/core/kernel/directory_operations.cpp | |||
src/core/kernel/idle_thread.cpp src/core/kernel/timers.cpp | |||
src/core/kernel/fcram.cpp |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fcram.hpp also needs to be added to the HEADER_FILES list otherwise Visual Studio's filters don't list it.
include/kernel/fcram.hpp
Outdated
|
||
class Memory; | ||
|
||
typedef std::list<FcramBlock> FcramBlockList; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
using FcramBlockList = std::list
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this file also could use a quick clang-format run
struct Region { | ||
struct Block { | ||
s32 pages; | ||
s32 pageOffs; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Recommend changing offs to offset
@@ -139,8 +161,8 @@ class Memory { | |||
static constexpr u32 DSP_DATA_MEMORY_OFFSET = u32(256_KB); | |||
|
|||
private: | |||
std::bitset<FCRAM_PAGE_COUNT> usedFCRAMPages; | |||
std::optional<u32> findPaddr(u32 size); | |||
//std::bitset<FCRAM_PAGE_COUNT> usedFCRAMPages; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should probably be deleted since they're totally superseded
src/core/kernel/fcram.cpp
Outdated
void KFcram::reset(size_t ramSize, size_t appSize, size_t sysSize, size_t baseSize) { | ||
fcram = mem.getFCRAM(); | ||
refs = std::unique_ptr<u32>(new u32[ramSize >> 12]); | ||
memset(refs.get(), 0, (ramSize >> 12) * sizeof(u32)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
std::memset
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this file can also use a quick clang-format, seems like headers at the top at least are unformatted
The existing memory management system is somewhat dodgy, to put it lightly. This PR aims to rewrite it almost entirely to improve overall accuracy of the emulator. This is currently a work in progress and will require quite a bit of time to implement all the planned features.
What needs to be done next (in no particular order):