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

arm64: fixes around icache flushing #4204

Merged
merged 2 commits into from Sep 10, 2016

Conversation

@lewurm
Copy link
Contributor

commented Sep 9, 2016

The first hit on Google about "Exynos 8890 SIGILL" is a thread pointing out a dolphin crash on Samsung Galaxy S7. We had a similar issue in mono, see mono/mono#3549

Untested though: Anyone cares about testing it please?


This change is Reviewable

@hrydgard

This comment has been minimized.

Copy link
Contributor

commented Sep 9, 2016

So THAT's what it is, thank you! Same issue in PPSSPP, we "solved" it with terrifying amounts of padding.

#endif
/* Don't rely on GCC's __clear_cache implementation, as it caches
* icache/dcache cache line sizes, that can vary between cores on
* big.LITTLE architectures. */

This comment has been minimized.

Copy link
@PatrickFerry

PatrickFerry Sep 9, 2016

Contributor

Can you change the comments to use single line comments instead (//) to match dolphin's style guide.

This comment has been minimized.

Copy link
@lewurm

lewurm Sep 9, 2016

Author Contributor

sure! :)

@lewurm lewurm force-pushed the lewurm:arm64-icache-big-little-fix branch from e7acee0 to 8f4f8a4 Sep 9, 2016

@skidau

This comment has been minimized.

Copy link
Contributor

commented Sep 9, 2016

@lewurm lewurm force-pushed the lewurm:arm64-icache-big-little-fix branch from 8f4f8a4 to 0248ace Sep 9, 2016

@lewurm

This comment has been minimized.

Copy link
Contributor Author

commented Sep 9, 2016

Thanks @skidau, fixed it up.

@PatrickFerry

This comment has been minimized.

Copy link
Contributor

commented Sep 10, 2016

@lewurm lewurm force-pushed the lewurm:arm64-icache-big-little-fix branch from 0248ace to ce87586 Sep 10, 2016

@lewurm

This comment has been minimized.

Copy link
Contributor Author

commented Sep 10, 2016

fixed lint issues. Great CI setup btw!

@skidau

This comment has been minimized.

Copy link
Contributor

commented Sep 10, 2016

Tested this out and it works for me on a S7 Exynos.

@lewurm lewurm force-pushed the lewurm:arm64-icache-big-little-fix branch from ce87586 to fff8221 Sep 10, 2016

@lewurm

This comment has been minimized.

Copy link
Contributor Author

commented Sep 10, 2016

okay, one more try on the linter :)


addr = (u64)start & ~(u64)(dsize - 1);
for (; addr < (u64)end; addr += dsize)
__asm__ volatile("dc civac, %0" : : "r"(addr) : "memory");

This comment has been minimized.

Copy link
@degasus

degasus Sep 10, 2016

Member

The manual suggests CVAU instead of CIVAC. I don't think we'll read this memory again, but I neither see a reason to invalidate the dcache. Was this done on purpose?

https://people.mozilla.org/~sstangl/arm/AArch64-Reference-Manual.pdf page 73

This comment has been minimized.

Copy link
@lewurm

lewurm Sep 10, 2016

Author Contributor

hum, I should have documented that better here as well. Reason for CIVAC is a suggested workaround by a couple of Cortex A-53 erratas, see here: v8/v8@fec99c6

@degasus

This comment has been minimized.

Copy link
Member

commented Sep 10, 2016

IminLine: Log2 of the number of words in the smallest cache line of all the instruction caches that are
controlled by the processor.

Not sure about processor != core, but this might still be a Exynos bug.

Also no clue which one is better, CVAU or CIVAC. Both LGTM.

@degasus

This comment has been minimized.

Copy link
Member

commented Sep 10, 2016

Reviewed 1 of 1 files at r3.
Review status: all files reviewed at latest revision, 1 unresolved discussion.


Comments from Reviewable

@degasus degasus merged commit 077fa09 into dolphin-emu:master Sep 10, 2016

10 of 11 checks passed

code-review/reviewable 1 discussion left (ZephyrSurfer)
Details
default Very basic checks passed, handed off to Buildbot.
Details
lint Build succeeded on builder lint
Details
pr-android Build succeeded on builder pr-android
Details
pr-deb-dbg-x64 Build succeeded on builder pr-deb-dbg-x64
Details
pr-deb-x64 Build succeeded on builder pr-deb-x64
Details
pr-freebsd Build succeeded on builder pr-freebsd
Details
pr-osx-x64 Build succeeded on builder pr-osx-x64
Details
pr-ubu-x64 Build succeeded on builder pr-ubu-x64
Details
pr-win-dbg-x64 Build succeeded on builder pr-win-dbg-x64
Details
pr-win-x64 Build succeeded on builder pr-win-x64
Details
@degasus

This comment has been minimized.

Copy link
Member

commented Sep 10, 2016

Thanks for sharing the outcome :D

hrydgard added a commit to hrydgard/ppsspp that referenced this pull request Sep 10, 2016
Port over the Exynos cacheline size fix from Dolphin. Thanks to lewur…
…m of the mono project for the discovery and original fix.

See dolphin-emu/dolphin#4204 and mono/mono#3549
@dolphin-emu-bot

This comment has been minimized.

Copy link
Contributor

commented Sep 10, 2016

FifoCI detected that this change impacts graphical rendering. Here are the behavior differences detected by the system:

  • rs3-bumpmapping on dx-win-nv: diff

automated-fifoci-reporter

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
6 participants
You can’t perform that action at this time.