GeckoCodes: Don't run PPC code in CoreTiming callbacks #4216

EmptyChaos · 2016-09-15T02:40:12Z

Gecko::RunCodeHandler uses PowerPC::SingleStep inside a CoreTiming callback, this results in a recursive call to CoreTiming::Advance which doesn't make sense. Advance is not designed to be re-entrancy safe and we're technically inside the slice boundary between two blocks while trying to run sub-slices inside the previous boundary.

It seems that the Interpreter contains a hack specifically to deal with this situation by overriding the slice length through the exposed global variable. Unfortunately, I accidentally broke that in PR #4201 so Gecko Codes no longer work correctly (The handler is causing 20000 cycle forward time skips in CoreTiming).

I'm not sure what exactly the code was trying to accomplish that couldn't be accomplished by just overwriting PC and NPC with the code handler entrypoint which is what I've changed it to do. [Someone more familiar with the JIT should double check this]

There was also a race condition with s_active_codes and the code table builder had a weird flaw where it writes half of a code then truncates the rest if it won't fit.

This change is

Korados · 2016-09-22T20:48:39Z

Reviewed 2 of 2 files at r1.
Review status: all files reviewed at latest revision, all discussions resolved.

Comments from Reviewable

Source/Core/Core/GeckoCode.cpp

+  // the original return address (which will still be in the link register) at the end.
+  if (PC == LR)
+  {
+    PC = NPC = INSTALLER_BASE_ADDRESS + 0xA8;


Source/Core/Core/GeckoCode.cpp


-    if (PC == LR)
+    if (!s_code_handler_installed || PowerPC::HostRead_U32(INSTALLER_BASE_ADDRESS) - 0xd01f1bad > 5)


degasus · 2016-09-23T10:06:57Z

Code looks good to me. I'm not 100% sure if the functional change in PC= instead of calling the interpreter is fine, but I'm fine with testing this on master.

Reviewed 2 of 2 files at r1.
Review status: all files reviewed at latest revision, 2 unresolved discussions.

Comments from Reviewable

phire

So I checked, and found the reason why the original code called interpreter inside Advance.

It's manipulating MSR and needs to set it back after the code handlers return.

You need to fix this evil TODO before the behavior will be correct.

Source/Core/Core/GeckoCode.cpp


-    if (PC == LR)
+    if (!s_code_handler_installed || PowerPC::HostRead_U32(INSTALLER_BASE_ADDRESS) - 0xd01f1bad > 5)


Source/Core/Core/GeckoCode.cpp

+  // If the last block that just executed ended with a BLR instruction then we can intercept it and
+  // redirect control into the Gecko Code Handler. The Code Handler will automatically BLR back to
+  // the original return address (which will still be in the link register) at the end.
+  if (PC == LR)


phire · 2016-09-28T22:14:34Z

Much better.

But I can't help but wonder if catching random BLRs is a bad idea. It sounds like the gecko codehandler has a few SDK functions that it likes to patch, which can be assumed to always be following the ABI.

But what if we hit a BLR in the middle of some handcoded assembly? The floating point registers might not be volatile anymore.

It might be a good idea to always use the new HLE trampoline. Also, you might want to consider dealing with the possibility that SP isn't sane.

EmptyChaos · 2016-09-29T06:04:15Z

@phire This is still a WIP which is why I didn't post a message when I pushed the current changes. My local branch is significantly different to the current PR code, I'm still iterating on the problem.

But what if we hit a BLR in the middle of some handcoded assembly? The floating point registers might not be volatile anymore.

I'm not sure I follow. Volatility refers to registers that are caller-saved vs callee-saved. Volatile registers are saved by the caller, non-volatile by the callee. FPR0-13 need to be saved because we're about to function call so we must caller-save them, the handler itself never uses FPR14-31 unless it calls out into the game code, the game function would then be responsible for saving FPR14-31 as part of the ABI so we don't need to handle that.

Also, you might want to consider dealing with the possibility that SP isn't sane.

This is something I don't have a good solution for. The obvious thing to do is to just create a new stack specifically for this purpose and destroy it afterwards. This requires choosing an address in RAM which is always safe for the stack to go, and there doesn't seem to be any candidates. Creating a custom Dolphin specific RAM zone that doesn't exist in real hardware for this purpose is possible but then the DBAT and page tables get in the way.

We could estimate if the stack is sane by walking it back to the root and seeing if we get a nonsense frame pointer in the top 2 frames but that isn't completely reliable. It also wouldn't help with the Red Zone problem where assembly code writes underneath the stack pointer and tries to pass that between functions.

But I can't help but wonder if catching random BLRs is a bad idea. It sounds like the gecko codehandler has a few SDK functions that it likes to patch, which can be assumed to always be following the ABI.

I agree, but a fixed dedicated hook point needs to satisfy several specific properties:

It needs to be called at least once every VI field, but not too often otherwise it'll kill performance
It needs to adhere to the ABI
It needs to work in every game

The main problem I'm having is that totaldb.dsy only works for GameCube games. For example, if I wanted to install a hook by HLE replacing VISetNextFrameBuffer (standard Gecko Hook 1 which works for 90% of games), that function never shows up in Wii games using "Symbols > Generate Symbol Map" because the Wii SDK signature for that function is missing from the database. The database seems to be Dolphin specific, I have no idea how to update it.

Gecko OS works by doing a memmem for certain instruction sequences which occur uniquely in the function it wants to hook then scans for the first occurrence of 0x4E800020 (BLR instruction) which gets replaced by a B 0x800018a8 hook. This scan can whiff entirely requiring the user to configure a different hook type or invent a custom game-specific hook search sequence to be used. In Dolphin, that would mean the game INIs would need to do something like

[Core]
GeckoHookSymbol=VISetNextFrameBuffer
; mr r3, r7; addi r4, r7, 52; addi r5, r7, 56; addi r6, r7, 76
GeckoHookSignature=0x7CE33B78,0x38870034,0x38A70038,0x38C7004C
GeckoHookSignatureMasks=0xFFFFFFFF,0xFFFFFFFF,0xFFFFFFFF,0xFFFFFFFF

Gecko OS Hooks:

VISetNextFrameBuffer (Default hook)
Common GC pad handling instruction sequence in game code [non-SDK] (unreliable)
Common Wiimote handling instruction sequence in game code [non-SDK] (unreliable)
GXDraw
GXFlush
OSSleepThread
"AXNextFrame"
Custom instruction pattern, up to 8 words/instructions

I'll try porting the Gecko OS system into Dolphin.

mimimi085181 · 2016-09-29T06:38:04Z

I'm for patching the executables the same way as Gecko OS does. This would make the codes run the same way they do on real hardware. So, if Dolphin emulates the hardware well enough, it would have the same compatibility, issues and quirks the real hardware has with the gecko codes. Dolphin's way of running the code handler might be better, but then you can't develop codes in Dolphin and expect them to work on real hardware.
(sorry if this is stating the obvious)

The hooks you list are only the wii hooks. In the neogamma source code, you'll find a bunch of other hooks for gamecube games, but ideally, you'd want to find out which hook gecko os for gamecube uses. Most old gecko codes from gamecube should be written for that hook.

Other hooks:
https://github.com/crediar/sneek/blob/master/di/dip.c#L864
I think those are wii hooks.

https://github.com/crediar/diosmios/blob/bc3f70601b189944a0f28d976624cba6fae7a404/Patches.c#L750
This should be a useable hook for gamecube games.

Gecko OS also installs a multidol hook, which patches other .dol files that are loaded via es launch. Gecko OS always uses the same hook type for main.dol and other .dol files, but you could use another here. I'm not sure which is better: Use the same multidol hook, or to patch loaded executables ourselves. I think patching ourselves might be better, since we also want this for gamecube games with other executables. On gamecube there are weird containers with .dol files, that are basically iso files(Ocarina of Time should be one example), and .elf files(James Bond games).

JMC47 · 2016-09-30T13:49:52Z

Regardless of if there's a better solution, we kind of need to fix this, it's a big problem.

EmptyChaos · 2016-10-01T02:31:39Z

I've polished the code in this branch instead of pushing the more complicated one. This can be merged as is. I can open a new PR for a rewrite later.

I've incorporated phire's suggestion to probe if the stack is sane, and to always use the trampoline in case of hand-rolled assembler that doesn't follow the ABI correctly.

JMC47 · 2016-10-01T03:51:33Z

I'm happy with this getting merged ASAP

On Fri, Sep 30, 2016 at 10:31 PM, EmptyChaos notifications@github.com
wrote:

I've polished the code in this branch instead of pushing the more
complicated one. This can be merged as is. I can open a new PR for a
rewrite later.

I've incorporated phire's suggestion to probe if the stack is sane, and to
always use the trampoline in case of hand-rolled assembler that doesn't
follow the ABI correctly.

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#4216 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/AGSuQSMO-8YMsXqAbPkPSL-4W99xw0grks5qvcYUgaJpZM4J9cg8
.

shuffle2 · 2016-10-03T04:55:03Z

lgtm
plz2rebase

Executing PPC code inside an external events callback is a bad idea. CoreTiming::Advance does not support recursion properly which will cause timing glitches. The interpreter has a slice length hack that counters this but without it this would cause a 20000 cycles time skip. It isn't clear what this was supposed to accomplish that just changing the current PC would not. Changing the PC works fine.

The active codes vector cannot safely be used outside the mutex, move the lock out into RunCodeHandler. s_code_handler_installed was also racing against SetActiveCodes since it's being written both inside and outside the lock. General cleanup. Add s_ prefixes, use constexpr, remove C casts.

The code table builder cuts off the end of codes that won't fit after already writing part of it. That seems quite unlikely to work the way anyone would find useful since the codes can contain actual PPC instructions.

Turns out one of the magic numbers was very magic. The gameid is an ad-hoc comm protocol with HLE_Misc to control the number of times the ICache is reset.

If the installation fails because codehandler.bin is missing or unusable then Dolphin will try again every single frame even though it's highly unlikely a disk file will have changed. Better to just fail once then only try again when the active code set is changed. Suppresses generating 60 log messages per second.

Dolphin emulates GeckoCodes by fiddling with the CPU state when a VI Interrupt occurs. The problem with this is that we don't know where the PC is so it's non-deterministic and not necessarily suitable for use with the codehandler. There are two options: Patch the game like Gecko OS either directly or using HLE::Patch, or use a trampoline so we can branch from any PC even if it would otherwise not be valid. The problem with Gecko OS patches is there are 10 of them and they have to be configured manually (i.e. Game INIs to would need to have a [Core]GeckoHookType property). HLE_Misc::GeckoReturnTrampoline enables the Code Handler to be entered from anywhere, the trampoline restores all the registers that had to be secretly saved to the stack.

Instead of fiddling with the MSR value, just reschedule and try again after the game fixes it itself.

GeckoCodes require address hooks which don't correspond to any symbol in the symbol table. The hooks get deleted when repatching the game because they did not persist across calls to HLE::PatchFunctions.

Try to make sure the stack is sane before calling into the codehandler. This is intended to reduce the possibility of random memory corruption.

Because of the way this works, randomly overwriting the handler when loading a savestate will break things because of the self-modifying nature of the handler.

Korados · 2016-10-03T15:05:38Z

I'm very sorry to break in here, but I wanted to inform you that button-activated Gecko Codes still don't work. I really would have liked to see the Gecko Codes work like on a real Wii.

And I wanted to apologise that I just clicked on Reviewed earlier. I thought someone just forgot this and I wanted to help.

JMC47 · 2016-10-03T15:06:24Z

This is not a fix for that, it's just a fix for the regression of gecko codes not working. We may have an idea of what's wrong, but, there's no guarantee of that.

EmptyChaos · 2016-10-04T03:21:13Z

@Korados Button codes should work fine as long as they aren't instruction-modifying codes. The GCH doesn't invalidate the instruction cache so cheats that patch the game code in a conditional code won't stop the JIT recycling the already compiled block without the patch.

In Mario Kart Double Dash there's a "press Z to jump" cheat, that works for example.

Korados · 2016-10-05T00:21:21Z

I'm very sorry but I think I don't understand what you said. I have no idea about programming at all. I'm just the fool who clicked on reviewed because I thought somebody forgot it. I tried the Flytime code for Wii Sports Resort. It is supposed to freeze the timer, and when you press 1+2+B, the timer jumps to 0. The timer actually gets freezed at 5 minutes but pressing the buttons has no effect. I don't know if this is an instruction-modifying code.

lioncash · 2016-10-05T00:35:18Z

I have no idea about programming at all.

... clicked on reviewed because I thought somebody forgot it.

In the future please don't do this; especially if you don't know what you're reviewing. You can ask devs about the state of a PR on IRC.

JMC47 · 2017-01-07T11:30:34Z

This apparently has been breaking various games with MMU off for a while. While turning MMU on fixes it (without performance hit even...) on x64, Android still doesn't have good MMU support. Pikmin 2, Metal Gear Solid: Twin Snakes, and other titles appear to be affected. They crash at various points.

Source/Core/Core/PatchEngine.cpp

+    return false;
+
+  // Check the link register makes sense (that it points to a valid IBAT address)
+  auto insn = PowerPC::TryReadInstruction(PowerPC::HostRead_U32(next_SP + 4));


EmptyChaos mentioned this pull request Sep 18, 2016

Interpreter/CachedInterpreter/JitArm64: Fix CoreTiming::Advance usage #4201

Merged

EmptyChaos force-pushed the geckocodes-cleanup branch 3 times, most recently from d420ace to c1636c0 Compare September 21, 2016 07:10

degasus reviewed Sep 23, 2016

View reviewed changes

Source/Core/Core/GeckoCode.cpp

// the original return address (which will still be in the link register) at the end.

if (PC == LR)

{

PC = NPC = INSTALLER_BASE_ADDRESS + 0xA8;

This comment was marked as off-topic.

Sign in to view

degasus reviewed Sep 23, 2016

View reviewed changes

Source/Core/Core/GeckoCode.cpp

if (PC == LR)

if (!s_code_handler_installed || PowerPC::HostRead_U32(INSTALLER_BASE_ADDRESS) - 0xd01f1bad > 5)

This comment was marked as off-topic.

Sign in to view

This comment was marked as off-topic.

Sign in to view

phire requested changes Sep 24, 2016

View reviewed changes

EmptyChaos force-pushed the geckocodes-cleanup branch 4 times, most recently from 2facc79 to 7968920 Compare September 25, 2016 04:12

phire approved these changes Sep 28, 2016

View reviewed changes

EmptyChaos changed the title ~~GeckoCodes: Don't run PPC code in CoreTiming callbacks + cleanup~~ [WIP] GeckoCodes: Don't run PPC code in CoreTiming callbacks Sep 29, 2016

EmptyChaos force-pushed the geckocodes-cleanup branch from 7968920 to 6e6c298 Compare October 1, 2016 01:55

EmptyChaos changed the title ~~[WIP] GeckoCodes: Don't run PPC code in CoreTiming callbacks~~ GeckoCodes: Don't run PPC code in CoreTiming callbacks Oct 1, 2016

EmptyChaos force-pushed the geckocodes-cleanup branch from 6e6c298 to d99bd6d Compare October 1, 2016 02:41

EmptyChaos force-pushed the geckocodes-cleanup branch from d99bd6d to 212e97d Compare October 1, 2016 06:17

EmptyChaos added 4 commits October 3, 2016 16:26

GeckoCode: Don't truncate codes that won't fit

4fef9d8

The code table builder cuts off the end of codes that won't fit after already writing part of it. That seems quite unlikely to work the way anyone would find useful since the codes can contain actual PPC instructions.

GeckoCode: Use named constants in ICache flush hack

541a42a

Turns out one of the magic numbers was very magic. The gameid is an ad-hoc comm protocol with HLE_Misc to control the number of times the ICache is reset.

EmptyChaos added 6 commits October 3, 2016 16:27

PatchEngine: Handle MSR more cleanly

c3cef54

Instead of fiddling with the MSR value, just reschedule and try again after the game fixes it itself.

HLE: Differentiate Address hooks from Symbol hooks

31cf843

GeckoCodes require address hooks which don't correspond to any symbol in the symbol table. The hooks get deleted when repatching the game because they did not persist across calls to HLE::PatchFunctions.

PatchEngine/GeckoCode: Heuristic stack checks

b354787

Try to make sure the stack is sane before calling into the codehandler. This is intended to reduce the possibility of random memory corruption.

GeckoCode: Save installation state to savestates

09372a5

Because of the way this works, randomly overwriting the handler when loading a savestate will break things because of the self-modifying nature of the handler.

EmptyChaos force-pushed the geckocodes-cleanup branch from 212e97d to 09372a5 Compare October 3, 2016 05:28

shuffle2 merged commit fba6801 into dolphin-emu:master Oct 3, 2016

EmptyChaos deleted the geckocodes-cleanup branch October 3, 2016 05:56

lioncash mentioned this pull request Oct 3, 2016

GeckoCode: Provide operator== and operator!= overloads #4276

Merged

EmptyChaos mentioned this pull request Oct 3, 2016

HLE: Fix warnings #4277

Merged

degasus reviewed Jan 18, 2017

View reviewed changes

EmptyChaos mentioned this pull request Jan 23, 2017

PatchEngine: Fix potential crashing during stack probe #4738

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

GeckoCodes: Don't run PPC code in CoreTiming callbacks #4216

GeckoCodes: Don't run PPC code in CoreTiming callbacks #4216

EmptyChaos commented Sep 15, 2016 •

edited

Korados commented Sep 22, 2016

This comment was marked as off-topic.

This comment was marked as off-topic.

This comment was marked as off-topic.

degasus commented Sep 23, 2016

phire left a comment

This comment was marked as off-topic.

This comment was marked as off-topic.

This comment was marked as off-topic.

phire commented Sep 28, 2016

EmptyChaos commented Sep 29, 2016

mimimi085181 commented Sep 29, 2016

JMC47 commented Sep 30, 2016

EmptyChaos commented Oct 1, 2016

JMC47 commented Oct 1, 2016

shuffle2 commented Oct 3, 2016

Korados commented Oct 3, 2016

JMC47 commented Oct 3, 2016

EmptyChaos commented Oct 4, 2016

Korados commented Oct 5, 2016 •

edited

lioncash commented Oct 5, 2016

JMC47 commented Jan 7, 2017

This comment was marked as off-topic.

This comment was marked as off-topic.


		if (PC == LR)
		if (!s_code_handler_installed \|\| PowerPC::HostRead_U32(INSTALLER_BASE_ADDRESS) - 0xd01f1bad > 5)

GeckoCodes: Don't run PPC code in CoreTiming callbacks #4216

GeckoCodes: Don't run PPC code in CoreTiming callbacks #4216

Conversation

EmptyChaos commented Sep 15, 2016 • edited

Korados commented Sep 22, 2016

This comment was marked as off-topic.

This comment was marked as off-topic.

This comment was marked as off-topic.

degasus commented Sep 23, 2016

phire left a comment

Choose a reason for hiding this comment

This comment was marked as off-topic.

This comment was marked as off-topic.

This comment was marked as off-topic.

phire commented Sep 28, 2016

EmptyChaos commented Sep 29, 2016

mimimi085181 commented Sep 29, 2016

JMC47 commented Sep 30, 2016

EmptyChaos commented Oct 1, 2016

JMC47 commented Oct 1, 2016

shuffle2 commented Oct 3, 2016

Korados commented Oct 3, 2016

JMC47 commented Oct 3, 2016

EmptyChaos commented Oct 4, 2016

Korados commented Oct 5, 2016 • edited

lioncash commented Oct 5, 2016

JMC47 commented Jan 7, 2017

This comment was marked as off-topic.

This comment was marked as off-topic.

EmptyChaos commented Sep 15, 2016 •

edited

Korados commented Oct 5, 2016 •

edited