Skip to content
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

MMU: Fix SDR updates being silently dropped in some cases #9625

Merged
merged 2 commits into from Apr 7, 2021

Conversation

leoetlino
Copy link
Member

@leoetlino leoetlino commented Apr 5, 2021

While 6xx_pem.pdf §7.6.1.1 mentions that the number of trailing zeros in HTABORG must be equal to the number of trailing ones in the mask (i.e. HTABORG must be properly aligned), this is actually not a hard requirement. Real hardware will just OR the base address anyway. Ignoring SDR changes would lead to incorrect emulation.

Logging a warning instead of dropping the SDR update silently is a saner behaviour.

(Thanks to @booto for helping me understand this a bit better and to @JMC47 for testing)


The only reason the US version of Pokemon Box worked with Dolphin's faulty assumption is that the pagetable buffer is heap allocated and it happens to be located at 0xbc0000 on that version. 0xbc0000 >> 16 = 0xbc is even so the alignment check would pass.

On the JP version, the pagetable buffer is found at 0xbf0000, which does not pass the check. (Side note: yes, this means that only half of the pagetable buffer is actually used. 0xc00000-0xc10000 is full of zeros.)


Real hardware doesn't appear to mask HTABORG or SDR:

  • If it actually used 0xbe instead of 0xbf as the value of HTABORG, the JP version wouldn't work on console either, because there are only zeros and no valid pagetable entries at 0xbe0000-0xbf0000...

  • ...assuming it configures the page tables by using the 0xbf0000 address. But the JP version doesn't use the address of the buffer it allocated. Instead it re-derives the pagetable base address and the addresses of all PTEs by reading back SDR and doing the following:
    image
    Assuming that real hw masked SDR, this would cause the game to clobber whatever is at 0xbe0000-0xbf0000, but theoretically the game would run fine if that memory area happens to be unallocated.

    In practice masking SDR in Dolphin makes the game clobber that memory area (as expected) but also crashes the game so that can't be what is happening on console.

Based on those observations, I am reasonably sure that real hardware doesn't actually mask HTABORG to fix its alignment.

In this situation, HTABORG = 0xbf and HTABMASK = 0x1 causes the second half of the pagetable to be aliased to the first half. This also seems to be the expected behavior because:

  • The game's init code (shown above) calculates PTE addresses by doing an OR rather than an ADD, and the second half of the table is completely blank.
  • OR'ing would be simpler than doing an addition for the hardware.
  • It also matches the behavior described in 6xx_pem.pdf Figure 7-21:

Figure 7-21

Remaining mysteries:

  • What happens if HTABMASK is invalid (contains 0s at the end)? My gut feeling is that real hw would just blindly AND just like how it blindly ORs, but we would need a hwtest to be sure.
  • What happens if the reserved bits in SDR aren't zero? 6xx_pem.pdf suggests that a MCE can occur but again I don't know what actually happens on a real Wii. (Note that Dolphin completely ignores the reserved bits, both prior to and after this PR.)

Perhaps those should be addressed in a followup PR.

@JMC47
Copy link
Contributor

JMC47 commented Apr 6, 2021

Fixes Pokemon Box Adventure Mode in JP and European. Refer to Issue 10500

image

GPXP01_2021-04-05_20-07-30

GPXP01_2021-04-05_20-07-36

@JMC47
Copy link
Contributor

JMC47 commented Apr 6, 2021

As a note, this does not work in any backup loaders as far as I know. According to issue reports, Freeloader does not boot it, Nintendont can boot it but the GBA games don't load even when using real disc + GBA games. Dios Mios is the same way according to users.

We believe that this may be because the game doesn't actually transfer the GBA game from the GBA as it "displays" in game. It has the GBA games on the disc already and just checks for which game it should load on the GBA itself. It uses Page Tables to put the GBA rom at 0x90000000, which is a rather odd address to use.

While I do not know how any of these backup loaders work, any BAT or memory mapped to 0x90000000 would likely override what the game is loading.

@leoetlino
Copy link
Member Author

The GBA game likely gets loaded into some buffer in MEM1 correctly but in Wii mode there's probably a BAT that maps 0x90000000 to MEM2, which conflicts with the game's page table entry for the ROM. BATs take precedence over PTEs so the game probably ends up reading and writing from the wrong location, even though the ROM loaded correctly.

@leoetlino
Copy link
Member Author

FYI: the only reason the US version worked with Dolphin's faulty assumption is that the pagetable buffer is heap allocated and it happens to be located at 0xbc0000 on that version. 0xbc0000 >> 16 = 0xbc is even so the alignment check would pass.

On the JP version, the pagetable buffer is found at 0xbf0000, which does not pass the check. (Side note: yes, this means that only half of the pagetable buffer is actually used. 0xc00000-0xc10000 is full of zeros.)

While 6xx_pem.pdf §7.6.1.1 mentions that the number of trailing
zeros in HTABORG must be equal to the number of trailing ones
in the mask (i.e. HTABORG must be properly aligned), this is actually
not a hard requirement. Real hardware will just OR the base address
anyway. Ignoring SDR changes would lead to incorrect emulation.

Logging a warning instead of dropping the SDR update silently is a
saner behaviour.
The swaps are confusing and don't accomplish much.

It was originally written like this:

u32 pte = bswap(*(u32*)&base_mem[pteg_addr]);

then bswap was changed to Common::swap32, and then the array access
was replaced with Memory::Read_U32, leading to the useless swaps.
@JMC47
Copy link
Contributor

JMC47 commented Apr 7, 2021

This is tested and shouldn't affect any working games as they would have been broken.

@JMC47 JMC47 merged commit 5322256 into dolphin-emu:master Apr 7, 2021
10 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
3 participants