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

Kernel: reimplement memory management on physical FCRAM #4392

Merged
merged 8 commits into from Nov 6, 2018

Conversation

Projects
None yet
@wwylele
Copy link
Member

wwylele commented Nov 1, 2018

This PR consists several changes that are chained together, as listed below

  • Change Memory::GetPhysicalPointer(LinearHeap) to access a unified FCRAM memory chunk directly
    • This is how it works on hardware, and eliminates an awkward dependency between video_core and kernel
  • Reimplement linear heap by allocating on the FCRAM
    • because video_core now uses FCRAM to access linear heap
  • Reimplement TLS page allocation on FCRAM
    • because linear heap is reimplemented
  • Reimplement SharedMemory to use raw backing pointer (list)
    • because linear heap is now backed by raw pointer
  • Reimplement regular heap by allocating on the FCRAM as well
    • because SharedMemory, which now uses raw pointer, can also be backed by regular heap
  • Allocate code segments (.text/.rodata/.data/.bss) and stack on FCRAM as well
    • because SharedMemory can also be backed by these segments. This is also how it is done on hardware.
  • Report memory usage to ResourceLimit::current_commit
    • because now we uses more FCRAM space than before. Some games use this value to exhaust the rest of FCRAM, so we need to correctly report how much memory is left.
  • Implement ControlMemory(MAP/UNMAP)
    • because now game can do MAP after deliberately exhausting the FCRAM, stubbing MAP with allocate would fail.

Sorry that I can't split the commit according to this list, as they all depend on each other, and any intermediate commit would render the software unusable.

The original goal was part of global state elimination of the memory module, and decoupling kernel and video_core was part of it. However, this now turns into reimplementation of many parts in the memory system. As this touches the foundation of the entire system, it might break or fix games in many ways, so please do thorough test on all games with this PR as much as possible.

This code leaves a new global variable Memory::fcram. It will be then refactored into system state in the next Memory module refactor PR.

This code change also have some (positive or negative) impacts in different aspects:

  • It might have resolved some memory address inconsistency that was shown by cheat codes
    • and it might break existing cheat codes as well.
  • It can in theory fragments heap inside FCRAM (just like how it is on real physical FCRAM). This makes it harder to implement fastmem. However, games usually behaves well and allocate large chunk of continuous memory.
  • Memory::GetPointer is not safe any more due to the discontinuity of heap. Luckily, there is only two uses of MemoryGetPointer in the entire codebase:
    • one in IPC, which is used to get command buffer pointer. This is with in one page, so it is still safe.
    • one in Y2R. It is theorized that Y2R only accepts linear heap address, which is always continuous, as it uses hardware/DMA for communication.
  • SharedMemory::GetPointer is not safe either as it can be backed by regular heap. This is annoying to resolve at once, so I only placed a warning inside SharedMemory::GetPointer when discontinuity is detected. Hopefully games usually behaves well and only allocates continuous heap.
  • Citra now always consumes constant 256MiB RAM for main game memory, instead of allocating by need.
  • It makes memory save states easier.
  • It opens the possibility of supporting hax or abnormal program behaviour that involves physical memory layout.
  • As memory aliasing is implemented, a following PR of implementing CRO on this (thus remove the ugly memory_synchronizer) will come.

A little review of 3DS memory system, to help with those who are new to this part of code:

FCRAM (128MiB for O3DS, or 256MiB for N3DS) is the main memory chip in 3DS for general purpose usage. It is divided into three regions: APPICATION, SYSTM and BASE. A program is allowed to allocate memory in one of the three region. (For example games are always in the APPLICATION region). Inside one region, there are two kinds of memory that can be allocated: (regular) heap and linear heap.

(Regular) heap is allocated starting from the end of the memory region, and growing down. They are mostly for application private use, and sometimes for software-based memory sharing (with other process using SharedMemory, for example). They can be mapped to anywhere inside 0x08000000~0x10000000 virtual memory range upon application request. When the application requests for a block of heap, the block is not guaranteed continuous in FCRAM and the location is not specified either.

Linear heap is allocated starting from the beginning of the memory region, and growing up. They are mostly for hardware-based memory sharing (with GPU, DMA etc.), but can also be used privately as well. They can be mapped to the linear heap virtual memory region (0x14000000+ or 0x30000000+, depending on game's kernel version). When the application requests for a linear heap block, the block is always continuous in FCRAM, and the difference between physical address and virtual address is always a constant. Therefore it gives VAddr->PAddr convertability, which enables communication with other hardware.


This change is Reviewable

@wwylele wwylele force-pushed the wwylele:fcram branch from d625a9a to 322064f Nov 1, 2018

@wwylele wwylele added the canary-merge label Nov 1, 2018

@FearlessTobi

This comment has been minimized.

Copy link
Contributor

FearlessTobi commented Nov 1, 2018

This PR fixes #2198 and #2660.

@valentinvanelslande

This comment has been minimized.

Copy link
Contributor

valentinvanelslande commented Nov 1, 2018

Fixes #2198
fixes21212

@FearlessTobi

This comment has been minimized.

Copy link
Contributor

FearlessTobi commented Nov 1, 2018

Reports of broken games so far: ORAS demo, MHXX.

@tywald

This comment has been minimized.

Copy link

tywald commented Nov 1, 2018

Fatal error when booting Etrian Odyssey Untold.
Tested on appveyor 20baca6 build. Doesn't happen on Nightly.

citra_log.txt

Edit*
Fixed.

@BreadFish64

This comment has been minimized.

Copy link
Contributor

BreadFish64 commented Nov 2, 2018

MHXX log from discord: https://pastebin.com/xs7KYu5Z

Kernel/Process: Unmap does not care the source memory permission
What game usually does is after mapping the memory, they reprotect the source memory as no permission to avoid modification there
@wwylele

This comment has been minimized.

Copy link
Member

wwylele commented Nov 2, 2018

Pushed a patch that should fix Etrian Odyseey Untold and ORAS demo. No idea about mhxx as I can boot it without problem.

@wwylele

This comment has been minimized.

Copy link
Member

wwylele commented Nov 2, 2018

As this PR can be updated very frequently and canary build might not catch up, please attach the canary version number, or the git hash in branch, when reporting issues.

@Krusher97

This comment has been minimized.

Copy link

Krusher97 commented Nov 2, 2018

MHXX only crashes when the translation patch is applied to it. I've only tried the v4 patch that was released before the release of GU.

The unpatched game boots just fine.

@tywald

This comment has been minimized.

Copy link

tywald commented Nov 2, 2018

MHXX only crashes when the translation patch is applied to it. I've only tried the v4 patch that was released before the release of GU.

The unpatched game boots just fine.

I'm gonna add that v1.4.0 update without modifications is working fine as well.

image

So it's just the translation patches then.

@wwylele

This comment has been minimized.

Copy link
Member

wwylele commented Nov 2, 2018

I'll log this here as no one has done yet:

Super Smash Bros is broken, with symptom of client-side out-of-memory. Investigating. I need two logs, from master and this branch, with Kernel.SVC:Debug level.

Kernel: fix a bug where code segments memory usage are accumulated twice
It is added to both misc and heap (done inside HeapAlloc), which results a doubled number reported by svcGetProcessInfo. While we are on it, we just merge the three number misc, heap and linear heap usage together, as there is no where they are distinguished.

Question: is TLS page also added to this number?
@wwylele

This comment has been minimized.

Copy link
Member

wwylele commented Nov 2, 2018

Pushed a commit that should fix Smash. I also find a new question: is TLS page memory usage reported by GetProcessInfo? Because I found that smash also reserves another unnecessary page when allocating memory, which can be only explained by the fact that we reports an additional page in GetProcessInfo caused by TLS allocation.

Will do more hwtest soon.

Edit: GetProcessInfo would return larger value on hw than citra (> heap+linear+segments+TLS), and spawning more threads would add to that value more than just TLS page. HW value likely also includes other stuff like thread context region, so citra isn't accurate anyway. I'll leave things as is.

@ghost

This comment has been minimized.

Copy link

ghost commented Nov 2, 2018

Fixes, Fire Emblem Warriors
#3018

@wwylele

This comment has been minimized.

Copy link
Member

wwylele commented Nov 2, 2018

Can anyone report whether the canary 1024 fixed smash and/or MHXX with english patch?

@MojoJojoDojo

This comment has been minimized.

Copy link

MojoJojoDojo commented Nov 2, 2018

Seeing it fixes a couple of New 3DS games i tried Runbow Pocket as well but it does not work as of right now.
There is no issue for the game btw.
@wwylele Smash works fine now.
UPDATE : opened an issue #4397.

@Krusher97

This comment has been minimized.

Copy link

Krusher97 commented Nov 2, 2018

MHXX with v4 translation patch still gives fatal error in 1024.
citra_log.txt

@Dragios

This comment has been minimized.

Copy link
Contributor

Dragios commented Nov 3, 2018

For archiving purposes:
Pokémon OR/AS Demo, Pokémon X/Y, Fire Emblem Fates, Fire Emblem Awakening generates this log and the game cannot continue (the only thing differs is their memory location)
Fixed with commit ff49785

Kernel <Info> core/hle/kernel/process.cpp:Unmap:368: Unmap memory target=0E000000, source=0883C000, size=00001000, perms=00000003
Service.ERR <Critical> core/hle/service/err_f.cpp:ThrowFatalError:154: Fatal error
Service.ERR <Critical> core/hle/service/err_f.cpp:ThrowFatalError:156: Fatal error type: Generic
Service.ERR <Critical> core/hle/service/err_f.cpp:LogGenericInfo:134: PID: 0x0000000B
Service.ERR <Critical> core/hle/service/err_f.cpp:LogGenericInfo:136: REV: 0x00000000_0x0000D798
Service.ERR <Critical> core/hle/service/err_f.cpp:LogGenericInfo:138: TID: 0x00000000_0x00000000
Service.ERR <Critical> core/hle/service/err_f.cpp:LogGenericInfo:140: AID: 0x00000000_0x00000000
Service.ERR <Critical> core/hle/service/err_f.cpp:LogGenericInfo:141: ADR: 0x001E0B10
Service.ERR <Critical> core/hle/service/err_f.cpp:LogGenericInfo:144: RSL: 0xE0A01BF5
Service.ERR <Critical> core/hle/service/err_f.cpp:LogGenericInfo:145:   Level: 28
Service.ERR <Critical> core/hle/service/err_f.cpp:LogGenericInfo:146:   Summary: 5
Service.ERR <Critical> core/hle/service/err_f.cpp:LogGenericInfo:147:   Module: 6
Service.ERR <Critical> core/hle/service/err_f.cpp:LogGenericInfo:148:   Desc: 1013
Service.ERR <Critical> core/hle/service/err_f.cpp:ThrowFatalError:167: Datetime: 2018/11/02 07:50:34

Super Smash Bros spams this log during battle: (Memory location differs each time and is random)
Certain maps will cause Citra emulation speed to reach almost halt (sound still play) and this also causes other program to crash. Which means total computer instability.

Fixed with commit dd899f4

HW.Memory <Error> core/memory.cpp:WriteBlock:556: unmapped WriteBlock @ 0x0000B700 (start address = 0x0000B700, size = 12156)
HW.Memory <Error> core/memory.cpp:Read:156: unmapped Read32 @ 0x00000000
HW.Memory <Error> core/memory.cpp:Read:156: unmapped Read8 @ 0x00000004

Full log: citra_log.zip


Pokémon Virtual Console titles generate this log during booting: (The game still runs as usual though)

Kernel <Error> core/hle/kernel/shared_memory.cpp:Map:158: Trying to map to already allocated memory
Kernel <Error> core/hle/kernel/shared_memory.cpp:Map:158: Trying to map to already allocated memory
Kernel <Error> core/hle/kernel/shared_memory.cpp:Map:158: Trying to map to already allocated memory
Kernel <Error> core/hle/kernel/shared_memory.cpp:Map:158: Trying to map to already allocated memory

Full log: https://pastebin.com/fgbEfwEv

@wwylele

This comment has been minimized.

Copy link
Member

wwylele commented Nov 3, 2018

Pokémon Virtual Console titles generate this log during booting:

@Dragios As you showed to me yesterday, the master branch also has four lines of error about not mapping the shared memory. This PR simply replaced the log with some different text, so it is essentially not related to the PR.

@wwylele

This comment has been minimized.

Copy link
Member

wwylele commented Nov 3, 2018

I just tried a MHXX 1.4+v5 english patch and it works for me on this branch... now I am so confused.

As the original author of the patch doesn't release CIA directly, I suspect that there are some bad CIA packagers who released some faulty CIA, which contains some corrupted info inside. To those who use english patch and experienced the issue, does the patch you used work on 3DS? If not, I would simply not care about these patches.

@Krusher97

This comment has been minimized.

Copy link

Krusher97 commented Nov 3, 2018

The v5 patch works according to others too. The only patch that seems to be crashing is the v4 one which was released before MHGU on Switch was released

@wwylele

This comment has been minimized.

Copy link
Member

wwylele commented Nov 4, 2018

Added a commit to log more info on SharedMemory mapping error (just like it did before).

@B3n30

B3n30 approved these changes Nov 5, 2018

// Allocate memory in heap
MemoryRegionInfo* memory_region = GetMemoryRegion(MemoryRegion::SYSTEM);
auto backing_blocks = memory_region->HeapAllocate(size);
ASSERT_MSG(!backing_blocks.empty(), "Not enough space in region to allocate shared memory!");

This comment has been minimized.

@B3n30

B3n30 Nov 5, 2018

Contributor

Does a 3DS panic on that too. Or is there some sort of "proper" error handling

This comment has been minimized.

@wwylele

wwylele Nov 5, 2018

Member

@B3n30 TBH I don't know. This ASSERT is copied from CreateSharedMemory, and the ASSERT there is inherited from the old code. Maybe @Subv know what error code should be returned from CreateSharedMemory. actually unrelated. CreateSharedMemoryForApplet represents two steps on real hardware: svcControlMemory(HeapAllocate) and then svcCreateMemoryBlock(CreateSharedMemory) on top of it. The error comes from the first operation.

However, I do think we should ASSERT in CreateSharedMemoryForApplet regardless of the correct error code, as this function is a shortcut for applets and is for internal use only. It is never exposed to guest program via SVC, so any error here directly means mistake in our own code.


// Try allocating from the higher address
for (auto iter = free_blocks.rbegin(); iter != free_blocks.rend(); ++iter) {
ASSERT(iter->bounds() == boost::icl::interval_bounds::right_open());

This comment has been minimized.

@jroweboy

jroweboy Nov 5, 2018

Member

Why is this assert needed?

This comment has been minimized.

@wwylele

wwylele Nov 5, 2018

Member

Not a single assert in the entire codebase is "needed" It is just for me to verify that I didn't do anything wrong in the code and that my understanding to boost::icl is correct.

* @param offset the region address offset to the beginning of FCRAM.
* @param size the size of the region to free.
*/
void Free(u32 offset, u32 size);

This comment has been minimized.

@jroweboy

jroweboy Nov 5, 2018

Member

The memory must have been allocated as heap or linear heap.

How does this handle heap in the following scenario:

[Heap A] [Linear B] [Heap A]

With my understanding of this Free (with offset + size) it seems like freeing Heap A would free Linear B as well. If so, is there a good reason we should document Free to work for heap allocations?

This comment has been minimized.

@jroweboy

jroweboy Nov 5, 2018

Member

I see now in Process::HeapFree you work around this limitation. Maybe, if possible, move the brunt of the HeapFree code into this class instead? Looking at it i don't think its really possible, but was just a thought.

This comment has been minimized.

@wwylele

wwylele Nov 5, 2018

Member

MemoryRegionInfo represents memory operation on physical space level, while Process::Allocate/Free translates memory operation from virtual space to physical space. It is not a workaround, but duty separation: the "continuous virtual block -> disjoint physical blocks" translation should happen in Process.

ResultVal<VAddr> Process::HeapAllocate(VAddr target, u32 size, VMAPermission perms) {
ResultVal<VAddr> Process::HeapAllocate(VAddr target, u32 size, VMAPermission perms,
MemoryState memory_state, bool skip_range_check) {
LOG_INFO(Kernel, "Allocate heap target={:08X}, size={:08X}", target, size);

This comment has been minimized.

@jroweboy

jroweboy Nov 5, 2018

Member

Does this really need to be INFO? looks much more like something suited for DEBUG

This comment has been minimized.

@FearlessTobi

FearlessTobi Nov 5, 2018

Contributor

From what I've seen, it isn't called that often and makes it easier to debug memory crashes.

This comment has been minimized.

@jroweboy

jroweboy Nov 5, 2018

Member

Memory crashes (after this is merged) will end up being extremely uncommon. If it's a memory crash, then we can change the log level to debug and still get the same messages.

VAddr interval_target = target;
for (const auto& interval : allocated_fcram) {
u32 interval_size = interval.upper() - interval.lower();
LOG_INFO(Kernel, "Allocated FCRAM region lower={:08X}, upper={:08X}", interval.lower(),

This comment has been minimized.

@jroweboy

jroweboy Nov 5, 2018

Member

Ditto, sounds like a DEBUG log instead of INFO

VAddr heap_end = GetLinearHeapBase() + (u32)linheap_memory->size();
// Games and homebrew only ever seem to pass 0 here (which lets the kernel decide the address),
// but explicit addresses are also accepted and respected.
LOG_INFO(Kernel, "Allocate linear heap target={:08X}, size={:08X}", target, size);

This comment has been minimized.

@jroweboy

jroweboy Nov 5, 2018

Member

here too

target + size < target) {
auto offset = memory_region->LinearAllocate(size);
if (!offset) {
LOG_ERROR(Kernel, "No enough space");

This comment has been minimized.

@jroweboy

jroweboy Nov 5, 2018

Member

"Not enough space"

return MakeResult<VAddr>(target);
}

ResultCode Process::LinearFree(VAddr target, u32 size) {
auto& linheap_memory = memory_region->linear_heap_memory;

LOG_INFO(Kernel, "Free linear heap target={:08X}, size={:08X}", target, size);

This comment has been minimized.

@jroweboy

jroweboy Nov 5, 2018

Member

ditto

}

ResultCode Process::Map(VAddr target, VAddr source, u32 size, VMAPermission perms) {
LOG_INFO(Kernel, "Map memory target={:08X}, source={:08X}, size={:08X}, perms={:08X}", target,

This comment has been minimized.

@jroweboy

jroweboy Nov 5, 2018

Member

d e b u g

ResultCode Process::Map(VAddr target, VAddr source, u32 size, VMAPermission perms) {
LOG_INFO(Kernel, "Map memory target={:08X}, source={:08X}, size={:08X}, perms={:08X}", target,
source, size, static_cast<u8>(perms));
if (source < Memory::HEAP_VADDR || source + size > Memory::HEAP_VADDR_END ||

This comment has been minimized.

@jroweboy

jroweboy Nov 5, 2018

Member

>

is this meant to be inclusive of the vaddr end? I recall earlier you used right open so maybe this should be >= ? (Don't change it if we are unsure, as that changes what the old did and i'd rather fix that bug later down the road instead of potentially introducing a new one with this)

This comment has been minimized.

@wwylele

wwylele Nov 5, 2018

Member

Both source + size and Memory::HEAP_VADDR_END represent right open boundaries. [A, C) can still be a subset of [B, C) (as long as A>=B) => should have no error when the two boundaries are equal => Use > for error condition.

return RESULT_SUCCESS;
}
ResultCode Process::Unmap(VAddr target, VAddr source, u32 size, VMAPermission perms) {
LOG_INFO(Kernel, "Unmap memory target={:08X}, source={:08X}, size={:08X}, perms={:08X}", target,

This comment has been minimized.

@jroweboy
@Subv

Subv approved these changes Nov 6, 2018

Copy link
Member

Subv left a comment

LGTM! Great job!


VAddr interval_end = std::min(target + size, vma->second.base + vma->second.size);
u32 interval_size = interval_end - interval_target;
u8* backing_memory = vma->second.backing_memory + (interval_target - vma->second.base);

This comment has been minimized.

@Subv

Subv Nov 6, 2018

Member

This is quite an ugly calculation, is there no way we can keep track of the FCRAM offset in the VMA object?

This comment has been minimized.

@wwylele

wwylele Nov 6, 2018

Member

The calculation looks fine to me :p . Yes we can modify VMA to support storing more information but I don't feel like going down potentially another rabbit hole...

This comment has been minimized.

@Subv

Subv Nov 6, 2018

Member

At the very least please refactor this into a separate function with a meaningful name, there's a few of places where you do this (here and SharedMemory) and it took me a while to get what it was doing at first.

static_cast<PAddr>(shared_memory->backing_block_offset);
shared_memory->backing_blocks = {{Memory::fcram.data() + *offset, size}};
shared_memory->holding_memory += MemoryRegionInfo::Interval(*offset, *offset + size);
shared_memory->linear_heap_phys_address = Memory::FCRAM_PADDR + *offset;

This comment has been minimized.

@Subv

Subv Nov 6, 2018

Member

The only use of this variable subtracts Memory::FCRAM_PADDR from it to retrieve the offset, you could just store the offset and do away with the extra clutter.

This comment has been minimized.

@wwylele

wwylele Nov 6, 2018

Member

It is also used by APT for PAddr->VAddr convertion, but I agree that storing offset sounds better (I have a follow up PR that can be simplified by this change)

@@ -23,6 +23,7 @@ namespace Memory {

static std::array<u8, Memory::VRAM_SIZE> vram;
static std::array<u8, Memory::N3DS_EXTRA_RAM_SIZE> n3ds_extra_ram;

This comment has been minimized.

@Subv

Subv Nov 6, 2018

Member

What is this 'extra ram' array used for now? isn't it already included in the whole FCRAM array?

This comment has been minimized.

@wwylele

wwylele Nov 6, 2018

Member

This array is a 0x00400000-long chip separate from FCRAM, located at PAddr=0x1F000000 and VAddr=0x1E800000, which is supposedly used by QTM.

@wwylele

This comment has been minimized.

Copy link
Member

wwylele commented Nov 6, 2018

Addressed comments above

@Subv

Subv approved these changes Nov 6, 2018

@wwylele wwylele merged commit 2067946 into citra-emu:master Nov 6, 2018

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@wwylele wwylele deleted the wwylele:fcram branch Nov 6, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment