SNES: CX4 cache and timing improvements

[AkiteruSDA]

• Remove extra cycle on bus access, is now 1+WS as expected
• When preloading cache by writing to $7F48, skip runtime checks and load the page immediately
• Program bank is set when writing to $7F48, as expected by MMX2/3's CX4 setup subroutine

These are the same changes as were recently merged into bsnes, SNES_MiSTer, and Mesen2RTA.

[Fiskbit]

Thanks for this PR! I'm not very familiar with SNES, but this code looks solid and it fixes the Mega Man X2 demo's boss fight, which desyncs without this change. I'm inclined to merge it in.

Can you give us permission to relicense this code in the future? Sour was working toward removing GPL content from Mesen with the goal of relicensing to something like MIT, so we'd like to ensure nontrivial submissions permit that.

[Fiskbit]

I found that this PR is triggering a nullptr exception in cx4test: https://snescentral.com/article.php?id=1116

I don't understand exactly why yet. It's happening the first time Cx4::ProcessCache is called and has the following stack trace:

MesenCore.dll!IMemoryHandler::GetMemoryType() Line 25	C++
MesenCore.dll!Cx4::GetAccessDelay(unsigned int addr) Line 254	C++
MesenCore.dll!Cx4::ProcessCache(unsigned __int64 targetCycle) Line 192	C++
MesenCore.dll!Cx4::Run() Line 81	C++
MesenCore.dll!BaseCartridge::SyncCoprocessors() Line 147	C++
MesenCore.dll!SnesMemoryManager::Exec() Line 221	C++
MesenCore.dll!SnesMemoryManager::IncMasterClock<4>() Line 138	C++
MesenCore.dll!SnesMemoryManager::Read(unsigned int addr, MemoryOperationType type) Line 272	C++
MesenCore.dll!SnesCpu::Read(unsigned int addr, MemoryOperationType type) Line 149	C++
MesenCore.dll!SnesCpu::ReadCode(unsigned short addr, MemoryOperationType type) Line 421	C++
MesenCore.dll!SnesCpu::GetOpCode() Line 381	C++
MesenCore.dll!SnesCpu::RunOp() Line 40	C++
MesenCore.dll!SnesCpu::Exec() Line 40	C++
MesenCore.dll!SnesConsole::RunFrame() Line 66	C++
MesenCore.dll!Emulator::Run() Line 141	C++
[External Code]	
MesenCore.dll!thread_start<unsigned int (__cdecl*)(void *),1>(void * const parameter) Line 97	C++
[External Code]	

ProcessCache is called with address 0. In this function, ReadCx4 gets a handler and avoids using that handler because it's nullptr, returning 0. However, the following GetAccessDelay gets the same nullptr handler and uses it regardless of its value, causing the exception. In the original code, these functions weren't reached at all on the first call to ProcessCache because it instead reaches the first return, which is now skipped because _state.Cache.Preload == true. I don't know which path through the first call to this function is actually correct.

If I add the same if(handler) check to GetAccessDelay, then cx4test runs and all of its tests pass. It makes sense that if one of these functions has this check, the other should, as well, but I don't understand why we're in this situation and if it means there's something else wrong.

[Fiskbit]

It looks like the cache is enabled by case 0x7F48 in Cx4::Write, and then on the next call to Run, ProcessCache gets run. It ends up working on address 0 because that's what it gets from (_state.Cache.Base + (_state.PB << 9)) & 0xFFFFFF. (I don't know if those variables have even been set by anything yet. I presume they haven't, since they're both 0.) There's apparently no mapping for address 0, so there's no handler.

I'm inclined to think that the new behavior is a step in the right direction because the old code was returning at a location commented with "Current cache page matches the needed address, keep using it", which doesn't seem plausible right when the cache is enabled.

The question, then, is what 'Populate the cache' should do when the address being read has no mapping.

[Fiskbit]

I've written a couple fixes that are committed on my branch of this repo, here: https://github.com/nesdev-org/MesenCE/commits/fiskbit-cx4-fixes/

One of them fixes the nullptr handler issue. The other fixes a desync of the Cx4State struct between the Core and UI; these need to be kept in sync or it can cause all sorts of problems with the debugging tools.

I tried to push these commits to your PR, but I don't seem to have permission. Can you pull them into this PR? We'll also still need permission from you to be able to relicense your commit in the future (such as to MIT).

With these changes, I don't know that the CX4 implementation is actually acting correctly in the situation it was previously crashing in, but it at least doesn't crash and passes all the available tests.

[AkiteruSDA]

Thank you for these fixes! I merged those into my branch. I think there's a possibility that cx4test itself is doing something illegal by not initializing the base cache address before triggering a preload, since MMX2/3 write non-zero values to 7F4A and 7F4B before ever triggering a preload. The fix is harmless though, so that sounds good to me.

I'm fine with relicensing as well, no worries there. I'm not sure why the Linux builds are failing. I could try merging the CE master into mine if rerunning them doesn't resolve the errors.

Edit: I just merged master into this branch, not sure if the workflows have to be run manually but hopefully that helps.