Two related upstream SA-1 bugs

[awjackson]

There are two related bugs with the SA-1 ROM mapping that apparently don't affect any original game, but are likely to cause problems with ROM-expanding ROM hacks sooner or later.

One bug is that vsprom.read() tests whether the remapped address is equal to the IRQ or NMI vector address, rather than the original bus address. If a game maps any ROM bank other than the first one to 00-1F, then the vector overriding for the S-CPU won't work.

Another bug is that the dynamic memory mapping (both ROM and BWRAM) isn't restored when you load a save state. I guess most (all?) commercial games.set the banks once at startup and never change them, so nobody ever noticed this bug, but it is very likely to bite ROM hacks.

The reason you have upstream reporting this issue downstream, instead of just fixing it, is that the only sane fix I can think of is to use a static address map with the remapping done in the memory handlers (like bsnes 075 and later) and that is definitely going to break your "cart usage map" feature. Anyway I'd like to abstract the usage mapping to more memory types (mirroring-aware usage maps for WRAM, cart RAM, and the BS-X PSRAM would be very useful) and I'd like to discuss with you the best way to do it.

[devinacker]

I'm thinking something like this for reworking the usage mapping (assuming none of this conflicts with how you want to change the memory mapping system):

• remove the cart_usage pointer from the CPU/chip debugger classes, and add a virtual method to the base Memory struct that returns a null pointer
• allocate usage maps in StaticRAM and MappedRAM, have them override said method and return usage map pointers
• have other Memory subclasses override said method in order to defer to any underlying physical memory (i.e. vsprom.usage() returns cartrom.usage())
• add usage flag get/set methods to the Bus struct that find and update the relevant usage map for a given bus address, if one exists

That would at least make the usage mapping honor ROM/RAM mirroring as much as possible and make it easier to get usage statistics for things other than the cart ROM.

[awjackson]

You must be a mind reader, because that's exactly what I have in mind, detail for detail. I guess I can just write the code in bsnes-classic and you will have little trouble integrating it, then.

Another thing I'd like to add is a "debugaccess" bool parameter to read(), write(), mmio_read() and mmio_write(), that handlers would check to disable unwanted side effects (e.g. switching threads) when the debugger accesses the bus. This is similar to how MAME/MESS handles debugger access to MMIO, and a big improvement on hardcoding no-touch address ranges into the debugger (as byuu originally did) or making a second copy of each bus with the side-effect-y handlers stripped out (which is what I believe you've done for the SuperFX)

With a few macros we should even be able to make it 100% performance impact free in non-debugger builds. Something like this:

#ifdef DEBUGGER
  #define MEMORY_READ(class) class::read(unsigned addr, bool debugaccess = false)
  #define MEMORY_WRITE(class) class::write(unsigned addr, uint8 data, bool debugaccess = false)
#else 
  #define MEMORY_READ(class) class::read(unsigned addr)
  #define MEMORY_WRITE(class) class::write(unsigned addr, uint8 data)
  enum : bool { debugaccess = false }
#endif

And in non-debug builds all the if(debugaccess)... clauses in the handlers should get optimized right out.

[devinacker]

If you want to go ahead and make those changes then, I can pull them in afterwards. I'd also like to move the original usage arrays out of all of the chip debuggers and into the bus struct, so that it could handle usage logging at both the bus level and memory level at the same time (which is useful for i.e. telling S-CPU code apart from SA-1 or SuperFX code, and so on).

The changes to the read/write functions sound good to me too. Definitely more flexible than having duplicate "safe" buses (which is indeed what I did for the SuperFX) and so on.

[awjackson]

On that front, I've been thinking that "read/write/execute" aren't the most useful categories for usage mapping, and we should perhaps use different categories per memory type. Something like the following: For all memory on the S-CPU bus (these are dictated by the disassembler):

S-CPU Opcode S-CPU Operand S-CPU M/X flags

Then, for MappedRAM objects: Coprocessor Opcode Coprocessor Operand S-CPU Data Coprocessor Data

For WRAM: A-bus Data B-bus Data (access through $2180)

For APU RAM: S-SMP Opcode S-SMP Operand S-DSP Data S-DSP Sample (S-DSP read) S-DSP Echo Buffer (S-DSP write)

These changes would require modifying the 65816 disassembler so that it can be told to use the "coprocessor opcode/operand" bits for this SA1 (the M/X flags can be shared, it's unlikely the two CPUs would execute the same code with different flags)

Does the SuperFX disassembler need anything like M/X flags? If it does, we're in trouble since we've used all the bits in a byte...

[devinacker]

Fortunately the SuperFX doesn't have any instructions that change size based on the processor state, so we wouldn't need anything like the M/X flags there.

Marking CPU vs. coprocessor code/data would probably also obviate the need to have separate usage tracking per bus, which is good.

[neagix]

Another bug is that the dynamic memory mapping (both ROM and BWRAM) isn't restored when you load a save state.

Shouldn't it be rather easy to create an extension to save states that also stores this information? I know, it could then be hard to have this accepted upstream at snes9x/bsnes...but at least one can try. The only good aspect is that given the nature of the save state format it would be backward compatible, so little friction to start adoption.

[devinacker]

@awjackson Are you planning to make any of the aforementioned memory access / usage mapping changes on your end soon? I'm going to be rewriting the memory editor in bsnes-plus and it would be nice to have those improvements to go along with it.

[awjackson]

I should have the debugger-access flag working very soon. The usage arrays will take a bit longer. I just realized that another feature will have to be redesigned to work with per-memory-object usage arrays: caching the usage to disk. I see that what you do is load the bus usage array and then populate the ROM usage array from it, but that won't work correctly if the memory mapping is different from what it was when the usage was saved (again, it works by accident because almost no commercial SA-1 games actually change the banks)

Something very nice I noticed byuu did in bsnes 075 was have the emulator construct an abstract list of memory objects to be battery saved, rather than the UI having a hardcoded list for each of normal/BS-X/Sufami Turbo "modes". I'll probably backport that idea (I need to to get saving to work in Exhaust Heat 2 and the shogi game that uses the same Seta DSP) and extend it to use for debugger purposes (usage arrays, memory save/load) as well as battery saving.

If you're rewriting the memory editor, I have a feature request: can you make it display VRAM/OAM/CGRAM as 16-bit words (i.e. hex digits grouped in 4s with the MSB first), and with addresses that match the actual PPU registers rather than being multiplied by 2? I realize this will require some thunking between the debugger and the emulator at the moment because bsnes is full of assumptions that all memory consists of arrays of uint8, but that's something I'm already working on on the emulator side (treating CGRAM as bytes, in particular, is nonsense on stilts...)

Access breakpoints on PPU memory should also be on word addresses, but that's a relatively trivial change I can make on my own... just one I've been holding out on because it's rather confusing to have the hexeditor addresses not match the breakpoint addresses.

[devinacker]

Shouldn't be difficult to make the new memory editor be able to switch between addressing both bytes and words (and I'll need to make sure the modified VRAM viewer shows tile addresses correctly while I'm at it...)

[awjackson]

The debugger-access stuff is now in. I used a member variable in the Debugger class rather than adding an argument to read()/write(); the latter approach turned out to require excessive amounts of ugly macro-ization to support both debug and non-debug builds (I didn't realize just how many Memory::read()/write() overrides were trampolines to other Memory objects...)

I haven't attempted to "sanitize" all the mmio_read()/mmio_write() methods yet; for now, debugger access to those gets stopped in MMIOAccess::read()/write() (which is essentially the way it worked before, so there's no regression in debugger functionality)

The common Memory classes are going to get reshuffled next time I have time to work on bsnes-classic. Instead of StaticRAM and MappedRAM that both inherit directly from Memory, there will be StaticRAM, MappedRAM and MappedROM that inherit from a common SimpleMemory class. Instead of a "write protect" flag to distinguish RAM from ROM they will be separate classes, and the confusing use of MappedRAM::copy() to do two completely different things (one when loading ROMs, the other when loading battery RAM contents) will go away.

It shouldn't affect anything you've done, just giving you a heads up.

[devinacker]

Alright, cool. I merged your changes (and axed the "safe" superfx bus and write protect calls that shouldn't be necessary anymore).

I'd like to go ahead and implement the usage map changes we talked about, just for the sake of having them conform to address mirroring, but I'll probably wait for you to restructure the memory classes before doing that.

[devinacker]

A bit overdue, but since somebody brought it up again in #46 I went ahead and attempted to use debugger_access() to make MMIO safely viewable from the debugger (commit d308c1be1590aa0fe5d0c42e0d1e9f02fd1f434f). It gets "lazy" for a few registers (e.g. $4016/4017 won't show the debugger what will actually be read from the shift registers, so you just get partial open bus instead), but for the most part it should be useful.