Dynamic bat #4146

degasus · 2016-08-23T20:54:49Z

This is a fork of PR #1882, with a few small fixup commits.

This change is

degasus · 2016-08-23T21:01:17Z

@magumagu Are you fine with my additional patches?

JMC47 · 2016-08-23T21:22:23Z

On top of all of the fixes mentions in the other pull request, We Love Golf! and Summer Athletics 2009 are now booting. They were dcbz sensitive games, so, make up your own conclusion on what fixed them.

Source/Core/Core/PowerPC/Jit64/Jit_LoadStore.cpp

-  // Slow path: mask the address, then call the general-case code.
-  SwitchToFarCode();
-  SetJumpTarget(slow);
-  AND(32, R(RSCRATCH2), Imm32(~31));
  MOV(32, M(&PC), Imm32(jit->js.compilerPC));


JMC47 · 2016-08-23T23:05:26Z

Tested a bunch of games using Dynamic BATs as my main Dolphin build today and everything seems to be fine.

lioncash · 2016-08-24T03:08:36Z

Review status: 0 of 19 files reviewed at latest revision, 8 unresolved discussions.

Source/Core/Core/HW/Memmap.cpp, line 229 [r2] (raw file):

}

void UpdateLogicalMemory(u32* dbat_table)

This parameter can be a const pointer.

Source/Core/Core/HW/Memmap.cpp, line 236 [r2] (raw file):

  }
  logical_mapped_entries.clear();
  for (unsigned i = 0; i < (1 << (32 - PowerPC::BAT_INDEX_SHIFT)); ++i)

These can just be u32 to remain consistent with all the other variables nearby.

Source/Core/Core/HW/Memmap.cpp, line 244 [r2] (raw file):

      unsigned logical_size = 1 << PowerPC::BAT_INDEX_SHIFT;
      unsigned translated_address = dbat_table[i] & ~3;
      for (PhysicalMemoryRegion& physical_region : physical_regions)

This can be a const reference as far as I can tell.

Source/Core/Core/PowerPC/MMU.cpp, line 103 [r2] (raw file):

  } result;
  u32 address;
  bool Success() { return result <= PAGE_TABLE_TRANSLATED; }

This can be a const member function.

Source/Core/Core/PowerPC/MMU.cpp, line 158 [r2] (raw file):

}

u32 dbat_table[1 << (32 - BAT_INDEX_SHIFT)];

std::array

Source/Core/Core/PowerPC/MMU.cpp, line 159 [r2] (raw file):

u32 dbat_table[1 << (32 - BAT_INDEX_SHIFT)];
u32 ibat_table[1 << (32 - BAT_INDEX_SHIFT)];

same here.

Source/Core/Core/PowerPC/MMU.cpp, line 395 [r2] (raw file):

  u32 hex;
  // TODO: Refactor this.

This should likely explain why it should be refactored, to provide context for anyone willing to do it.

Source/Core/Core/PowerPC/MMU.cpp, line 1186 [r2] (raw file):

static void UpdateFakeMMUBat(u32* bat_table, u32 start_addr)
{
  for (unsigned i = 0; i < (0x10000000 >> BAT_INDEX_SHIFT); ++i)

u32

Comments from Reviewable

degasus · 2016-08-24T06:10:08Z

Review status: 0 of 19 files reviewed at latest revision, 8 unresolved discussions.

Source/Core/Core/HW/Memmap.cpp, line 229 [r2] (raw file):

Previously, lioncash (Mat M.) wrote…

This parameter can be a const pointer.

Done.

Source/Core/Core/HW/Memmap.cpp, line 236 [r2] (raw file):

Previously, lioncash (Mat M.) wrote…

These can just be u32 to remain consistent with all the other variables nearby.

Done.

Source/Core/Core/HW/Memmap.cpp, line 244 [r2] (raw file):

Previously, lioncash (Mat M.) wrote…

This can be a const reference as far as I can tell.

Done.

Source/Core/Core/PowerPC/MMU.cpp, line 103 [r2] (raw file):

Previously, lioncash (Mat M.) wrote…

This can be a const member function.

Done.

Source/Core/Core/PowerPC/MMU.cpp, line 158 [r2] (raw file):

Previously, lioncash (Mat M.) wrote…

std::array

Done.

Source/Core/Core/PowerPC/MMU.cpp, line 159 [r2] (raw file):

Previously, lioncash (Mat M.) wrote…

same here.

Done.

Source/Core/Core/PowerPC/MMU.cpp, line 395 [r2] (raw file):

Previously, lioncash (Mat M.) wrote…

This should likely explain why it should be refactored, to provide context for anyone willing to do it.

Done.

Source/Core/Core/PowerPC/MMU.cpp, line 1186 [r2] (raw file):

Previously, lioncash (Mat M.) wrote…

u32

Done.

Comments from Reviewable

JMC47 · 2016-08-24T16:14:23Z

Because Twilight Princess doesn't suffer any kind of performance penalty from MMU being enabled (at least on my PC,) I've marked the dropped sign bug crash as fixed here.

Ebola16 · 2016-08-25T05:37:13Z

This PR has the same .elf loading issue as PR #1882

I just downloaded a build of this PR from https://dl.dolphin-emu.org/prs/pr-4146-dolphin-latest-x64.7z
to see if it has any effect on https://bugs.dolphin-emu.org/issues/8864 (SSBB Alloc Buffer failed).

When attempting to load Gecko OS's .elf Dolphin will crash (attached below). This problem does not occur for me on other builds of Dolphin (tested up to 5.0-443).

Win 10 Pro x64
Intel Core i7-4702MQ CPU @2.20GHz
GeForce GT 750M
2x 8GB Hynix 1600MHz
GeForce Game Ready Driver 372.54

1SSBB Gecko.zip

phire · 2016-08-25T13:49:57Z

Reviewed 6 of 20 files at r1, 3 of 6 files at r3.
Review status: 9 of 19 files reviewed at latest revision, 9 unresolved discussions.

Source/Core/Core/HW/Memmap.cpp, line 103 [r3] (raw file):

enum
{
  MV_FAKE_VMEM = 1,

MV Prefix (Memory View) is now incorrect as they are now used as flags for Physical Memory Regions.

Make this a named enum instead? Move it inside the PhysicalMemoryRegion struct?

Comments from Reviewable

phire · 2016-08-25T14:44:33Z

Reviewed 7 of 20 files at r1, 3 of 6 files at r3.
Review status: all files reviewed at latest revision, 11 unresolved discussions.

Source/Core/Core/HW/Memmap.cpp, line 233 [r3] (raw file):

  for (auto& entry : logical_mapped_entries)
  {
    g_arena.ReleaseView(entry.mapped_pointer, entry.mapped_size);

Should be be destroying and recreating the all the mappings for every single BAT change? Would it make sense to scan for entries which have changed?

mmaping isn't the cheapest of operations.

Source/Core/Core/PowerPC/PowerPC.h, line 288 [r3] (raw file):

static const int BAT_INDEX_SHIFT = 17;
using BatTable = std::array<u32, 1 << (32 - BAT_INDEX_SHIFT)>;

Could I request a "// 128KB" comment, so I don't have to pull out my calculator next time.

Assuming I even did the math correctly

Comments from Reviewable

degasus · 2016-08-29T16:59:01Z

Review status: 17 of 19 files reviewed at latest revision, 11 unresolved discussions.

Source/Core/Core/HW/Memmap.cpp, line 103 [r3] (raw file):

Previously, phire (Scott Mansell) wrote…

MV Prefix (Memory View) is now incorrect as they are now used as flags for Physical Memory Regions.

Make this a named enum instead? Move it inside the PhysicalMemoryRegion struct?

Done.

Source/Core/Core/HW/Memmap.cpp, line 233 [r3] (raw file):

Previously, phire (Scott Mansell) wrote…

Should be be destroying and recreating the all the mappings for every single BAT change? Would it make sense to scan for entries which have changed?

mmaping isn't the cheapest of operations.

It should, but I'd let this open for the next PR. Especially as no game is known to reconfigure the BATs more than once.

Source/Core/Core/PowerPC/PowerPC.h, line 288 [r3] (raw file):

Previously, phire (Scott Mansell) wrote…

Could I request a "// 128KB" comment, so I don't have to pull out my calculator next time.

Assuming I even did the math correctly

Done.

Comments from Reviewable

degasus · 2016-08-29T17:20:23Z

The last commit should fix ELF booting, but I haven't verified if our default mapping matches well.

Review status: 17 of 20 files reviewed at latest revision, 3 unresolved discussions.

Comments from Reviewable

JMC47 · 2016-08-29T17:21:16Z

Elf booting is not fully fixed. The DOL version of Gecko OS works, but the HBC version does not, unless I've already booted the dol version then it does for some reason.

lioncash · 2016-08-29T17:24:14Z

Review status: 17 of 20 files reviewed at latest revision, 5 unresolved discussions.

Source/Core/Core/PowerPC/MMU.cpp, line 756 [r5] (raw file):

  // Translate address
  u32 bat_result = dbat_table[address >> BAT_INDEX_SHIFT];

Note that this'll look pretty unusual to anyone new to the code (or anyone who has a grasp for on it, I guess), considering we have a TranslateAddress function, which, if inadequate as is, could just have an overload that accepts a BatTable and an address.

Source/Core/Core/PowerPC/MMU.cpp, line 778 [r5] (raw file):

  // Translate address
  u32 bat_result = dbat_table[address >> BAT_INDEX_SHIFT];

Same here

Comments from Reviewable

JMC47 · 2016-08-29T20:40:55Z

@Ebola16 Can you try Gecko OS? I'm having inconsistencies getting it to work.

JMC47 · 2016-08-29T21:01:13Z

SORRY! I compiled it myself and it works fine now. Whatever.

LGTM

Ebola16 · 2016-08-29T21:57:35Z

.elf files now load again!

phire · 2016-08-29T22:50:12Z

Reviewed 2 of 2 files at r4, 1 of 1 files at r5.
Review status: all files reviewed at latest revision, 4 unresolved discussions.

Source/Core/Core/HW/Memmap.cpp, line 233 [r3] (raw file):

Previously, degasus (Markus Wick) wrote…

It should, but I'd let this open for the next PR. Especially as no game is known to reconfigure the BATs more than once.

Next PR is fine.

Source/Core/Core/PowerPC/MMU.cpp, line 756 [r5] (raw file):

Previously, lioncash (Mat M.) wrote…

Note that this'll look pretty unusual to anyone new to the code (or anyone who has a grasp for on it, I guess), considering we have a TranslateAddress function, which, if inadequate as is, could just have an overload that accepts a BatTable and an address.

Yeah, might want to add DBAT_ONLY and IBAT_ONLY flags to `TranslateAddress` which terminate early.

Or just create TranslateBatAddess, considering we already have TranslatePageAddress

Source/Core/Core/PowerPC/MMU.cpp, line 778 [r5] (raw file):

Previously, lioncash (Mat M.) wrote…

Same here

And could we get a comment saying why we don't optimise for page translations?

Source/Core/Core/PowerPC/MMU.cpp, line 1232 [r5] (raw file):

template <const XCheckTLBFlag flag>
TranslateAddressResult TranslateAddress(const u32 address)
{

Could we get a comment saying BAT translations are defined by the PowerPC spec as taking priority over page translations, just so any new coders know what is going on here.

Comments from Reviewable

phire · 2016-08-29T22:55:18Z

Actually, now that I think about it...

Are IsOptimizableMMIOAccess and friends relying on on BAT translations of optimizable accesses to never change? Because now that we have dynamic BATs, they can change, which would require invalidating all jit blocks with optimises accesses.

magumagu · 2016-08-31T05:01:35Z

As you can probably guess from my lack of activity, I'm not going to be able to work on this in the near future.

The additional changes look fine.

Are IsOptimizableMMIOAccess and friends relying on on BAT translations of optimizable accesses to never change?

Yes.

Because now that we have dynamic BATs, they can change, which would require invalidating all jit blocks with optimises accesses.

DBatUpdated() calls JitInterface::ClearSafe(). Maybe worth a comment in the code, since this point has come up more than once.

degasus · 2016-09-03T12:59:02Z

Review status: all files reviewed at latest revision, 3 unresolved discussions.

Source/Core/Core/PowerPC/MMU.cpp, line 756 [r5] (raw file):

Previously, phire (Scott Mansell) wrote…

Yeah, might want to add DBAT_ONLY and IBAT_ONLY flags to TranslateAddress which terminate early.

Or just create TranslateBatAddess, considering we already have TranslatePageAddress

I've created a small inline function for this lines. I'd like to cleanup TranslateAddress later on.

Source/Core/Core/PowerPC/MMU.cpp, line 778 [r5] (raw file):

Previously, phire (Scott Mansell) wrote…

And could we get a comment saying why we don't optimise for page translations?

Done.

Source/Core/Core/PowerPC/MMU.cpp, line 1232 [r5] (raw file):

Previously, phire (Scott Mansell) wrote…

Could we get a comment saying BAT translations are defined by the PowerPC spec as taking priority over page translations, just so any new coders know what is going on here.

Done.

Comments from Reviewable

degasus · 2016-09-03T13:02:40Z

Review status: 15 of 20 files reviewed at latest revision, 4 unresolved discussions.

Source/Core/Core/PowerPC/MMU.cpp, line 1168 [r7] (raw file):

        // bit from the bottom is whether we can use the fastmem arena.
        u32 valid_bit = 0x1;
        if (Memory::bFakeVMEM && ((address & 0xFE000000) == 0x7E000000))

By the way, I dislike this 0x2 bit generation here a lot. I'd like to move it to IsOptimizableRAMAddress with physical_regions in mind. Do you think this shall be done now, or in a cleanup PR?

Comments from Reviewable

phire · 2016-09-04T07:57:58Z

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

Source/Core/Core/PowerPC/MMU.cpp, line 756 [r5] (raw file):

Previously, degasus (Markus Wick) wrote…

I've created a small inline function for this lines. I'd like to cleanup TranslateAddress later on.

Yeah, that looks fine.

Source/Core/Core/PowerPC/MMU.cpp, line 1168 [r7] (raw file):

Previously, degasus (Markus Wick) wrote…

By the way, I dislike this 0x2 bit generation here a lot. I'd like to move it to IsOptimizableRAMAddress with physical_regions in mind. Do you think this shall be done now, or in a cleanup PR?

It does seem a little weird to hardcode the physical mapping in here.

It's your choice really.

Comments from Reviewable

degasus · 2016-09-04T07:58:59Z

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

Source/Core/Core/PowerPC/MMU.cpp, line 1168 [r7] (raw file):

Previously, phire (Scott Mansell) wrote…

It does seem a little weird to hardcode the physical mapping in here.

It's your choice really.

We hardcode it in MMU.cpp a few times. Should be cleaned up, but I don't want to add big cleanups in the same PR.

Comments from Reviewable

Fundamentally, all this does is enforce the invariant that we always translate effective addresses based on the current BAT registers and page table before we do anything else with them. This change can be logically divided into three parts. The first part is creating a table to represent the current BAT state, and keeping it up to date (PowerPC::IBATUpdated, PowerPC::DBATUpdated, etc.). This does nothing by itself, but it's necessary for the other parts. The second part (mostly in MMU.cpp) is simply removing all the hardcoded checks for specific untranslated addresses, and consistently translating addresses using the current BAT configuration. Very straightforward, but a lot of code changes because we hardcoded assumptions all over the place. The third part (mostly in Memmap.cpp) is making the fastmem arena reflect the current BAT configuration. We do this by redoing the mapping (calling memmap()) based on the BAT table whenever it changes. One additional minor change is that translation can fail in two ways: either the segment is a direct store segment, or page table lookup failed. The difference doesn't usually matter, but the difference affects cache instructions, like dcbz.

degasus force-pushed the dynamic-bat branch from 60add40 to 8166c7d Compare August 23, 2016 20:57

phire reviewed Aug 23, 2016
View reviewed changes

degasus force-pushed the dynamic-bat branch from f898c50 to 493d869 Compare August 29, 2016 16:58

degasus force-pushed the dynamic-bat branch from 493d869 to 8672fce Compare August 29, 2016 17:00

degasus force-pushed the dynamic-bat branch from caeefad to 7da4d98 Compare September 3, 2016 12:58

degasus force-pushed the dynamic-bat branch 2 times, most recently from b3c3781 to a4d72ac Compare September 6, 2016 06:42

magumagu and others added 7 commits September 6, 2016 08:43

MMU: Small fixes.

ef0dcfe

Jit64: Simplify dcbz.

2925a5b

JitArm64: Rebase failure.

ae1421b

MMU: Coding style fixes.

6493ea1

Boot: Update BAT lookup table on booting ELF.

ce5dac1

MMU: Add small BAT comment.

a4d72ac

degasus merged commit dddd88a into dolphin-emu:master Sep 6, 2016

degasus mentioned this pull request Sep 6, 2016

[WIP] Dynamic bat #1882

Closed

4 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Dynamic bat #4146

Dynamic bat #4146

degasus commented Aug 23, 2016 •

edited by delroth

Loading

degasus commented Aug 23, 2016

JMC47 commented Aug 23, 2016

This comment was marked as off-topic.

This comment was marked as off-topic.

JMC47 commented Aug 23, 2016

lioncash commented Aug 24, 2016

degasus commented Aug 24, 2016

JMC47 commented Aug 24, 2016

Ebola16 commented Aug 25, 2016

phire commented Aug 25, 2016

phire commented Aug 25, 2016

degasus commented Aug 29, 2016

degasus commented Aug 29, 2016

JMC47 commented Aug 29, 2016

lioncash commented Aug 29, 2016

JMC47 commented Aug 29, 2016

JMC47 commented Aug 29, 2016

Ebola16 commented Aug 29, 2016

phire commented Aug 29, 2016

phire commented Aug 29, 2016

magumagu commented Aug 31, 2016

degasus commented Sep 3, 2016

degasus commented Sep 3, 2016

phire commented Sep 4, 2016

degasus commented Sep 4, 2016

Dynamic bat #4146

Dynamic bat #4146

Conversation

degasus commented Aug 23, 2016 • edited by delroth Loading

degasus commented Aug 23, 2016

JMC47 commented Aug 23, 2016

This comment was marked as off-topic.

This comment was marked as off-topic.

JMC47 commented Aug 23, 2016

lioncash commented Aug 24, 2016

degasus commented Aug 24, 2016

JMC47 commented Aug 24, 2016

Ebola16 commented Aug 25, 2016

phire commented Aug 25, 2016

phire commented Aug 25, 2016

degasus commented Aug 29, 2016

degasus commented Aug 29, 2016

JMC47 commented Aug 29, 2016

lioncash commented Aug 29, 2016

JMC47 commented Aug 29, 2016

JMC47 commented Aug 29, 2016

Ebola16 commented Aug 29, 2016

phire commented Aug 29, 2016

phire commented Aug 29, 2016

magumagu commented Aug 31, 2016

degasus commented Sep 3, 2016

degasus commented Sep 3, 2016

phire commented Sep 4, 2016

degasus commented Sep 4, 2016

degasus commented Aug 23, 2016 •

edited by delroth

Loading