jit-icache-coherence: clean the data cache before invalidating the instruction cache on aarch64 - #14454
jit-icache-coherence: clean the data cache before invalidating the instruction cache on aarch64#14454doracawl wants to merge 2 commits into
Conversation
…struction cache on aarch64 `clear_cache` on aarch64 only ran `ic ivau` over the range with a fixed 64-byte stride. On cores with CTR_EL0.IDC == 0 the dirty data cache line was never cleaned to the point of unification, so the invalidated instruction cache refetched the old bytes. Rewriting a function in a page that had already executed ran the previous function 180,425 times out of 200,000 on a Cortex-A72 (AWS a1.medium); the kernel only cleans a page the first time it becomes executable, which is why fresh pages hide this. Use the sequence compiler-rt's `__clear_cache` and the Linux kernel's `caches_clean_inval_pou_macro` use: `dc cvau` per DminLine unless IDC, `dsb ish`, `ic ivau` per IminLine unless DIC, `dsb ish`, `isb`, with the line sizes read from CTR_EL0 once. Reading CTR_EL0 at EL0 gets SIGILL on macOS, so that path calls `sys_icache_invalidate` like compiler-rt does on Darwin. Add a test that rewrites code in an executed page and checks the new code runs, and a unit test for the CTR_EL0 decoding. Fixes bytecodealliance#14442
cfallin
left a comment
cfallin
left a comment
There was a problem hiding this comment.
Thanks -- some questions below, but in general I am happy to see a more correct coherence implementation on non-Darwin aarch64!
| if ctr == 0 { | ||
| unsafe { | ||
| asm!("ic ivau, {}", in(reg) start); | ||
| asm!("mrs {}, ctr_el0", out(reg) ctr, options(nomem, nostack, preserves_flags)); |
There was a problem hiding this comment.
Does this cause any problems on big.LITTLE systems where different core types may exist? Or are they architecturally guaranteed to have the same cache coherence behavior? (Can you cite the relevant bit of the Arm ARM if so?)
There was a problem hiding this comment.
(By "does this cause problems" I mean specifically the caching, which is process-wide and survives across threads and across a single thread migrating between cores)
There was a problem hiding this comment.
Partly architectural. The Arm ARM's CTR_EL0 description (DDI 0487 M.d, D24.2.41) requires the two bits that pick the maintenance to agree: for DIC, "All PEs in the same Inner Shareable shareability domain must have a common value of this field", and for IDC, "... must have a common Effective value of IDC". The line sizes have no such rule. For those it's the OS: Linux (since 4.9, 116c81f427ff), NetBSD 10 and FreeBSD 15 trap EL0 reads of CTR_EL0 on mismatched systems and return one system-wide value with the smallest line sizes. Where the OS doesn't, re-reading on every call wouldn't help either, since the thread can migrate between the mrs and the loops; compiler-rt's __clear_cache caches it the same way.
On an RK3588 (4x A55 + 4x A76), both clusters report the same IDC, DIC and line sizes; they differ only in L1Ip.
I put the reasoning and the Arm ARM wording in a comment above the static in f29d643.
| } | ||
|
|
||
| #[cfg(target_arch = "aarch64")] | ||
| #[cfg(all(target_arch = "aarch64", target_vendor = "apple"))] |
There was a problem hiding this comment.
Is there a reason we can't use the existing implementation on aarch64-apple-darwin, since it comes from Darwin source (so is known to be correct on that platform) and avoids a call into system libraries?
There was a problem hiding this comment.
Yes: the Darwin source that sequence follows is the 2017 libplatform. The sys_icache_invalidate that ships today (libplatform-306 and later, macOS 14+) also issues an extra dsb ish after every 20 ic ivau on the CPU families in its cpus_that_need_dsb_for_ic_ivau table (cache.s#L31-L92); I checked the disassembly of libsystem_platform.dylib on macOS 27 and it matches. An inline copy would miss that and any later change. The 2017 version also has a dsb ish before the loop that the inline code doesn't (D7.5.9.15 makes ic ivau unordered against earlier stores without it).
Calling it adds no dependency, since std already links libSystem. Added this to the comment in f29d643.
…ll on Apple Say in the comments why reading CTR_EL0 once per process is safe: IDC and DIC must be the same on every core of an Inner Shareable domain (Arm ARM, CTR_EL0), and Linux, NetBSD 10 and FreeBSD 15 hand EL0 a uniform value for the line sizes. On Apple, keep calling `sys_icache_invalidate`: since macOS 14 it issues an extra `dsb ish` every 20 `ic ivau` on some CPU families, which an inline copy of the sequence would not. Move the per-line iteration into `cache_lines` with a unit test, and add CTR_EL0 decoding cases for the Cortex-A55, A64FX and what Linux reports on a Neoverse-N1 with erratum 1542419. prtest:macos-arm64 prtest:linux-arm64
cfallin
left a comment
cfallin
left a comment
There was a problem hiding this comment.
OK, the substance of this looks good now -- thanks for the persistence in finding sources!
The last thing I want to see cleaned up a bit is the "cfg soup" -- there are a lot of complex cfg conditions in this one file, and each top-level decl needs one; it'd be better if we split out per-system-config implementation modules and had one top-level cfg to use the module and re-export (pub use impl::*; kind of pattern). See e.g. here for a good example.
Once you do that refactor, I'm happy to merge; thanks!
This branch has not been deployed
2 participants
clear_cacheon aarch64 only issuedic ivauwith a fixed 64-byte stride and never cleaned the data cache to the point of unification, so on cores with CTR_EL0.IDC == 0 the invalidated instruction cache refetched the old bytes, the new ones still sitting in a dirty data cache line above the point of unification. Fresh pages hide this because the Linux kernel cleans a page the first time it becomes executable; rewriting code in a page that has already executed gets no such help, and that is exactly what a JIT reusing code memory does.This switches to compiler-rt's
__clear_cachesequence (https://github.com/llvm/llvm-project/blob/3390613ccf0fbbb40026dcbabb36fce444f79480/compiler-rt/lib/builtins/clear_cache.c#L121-L153; the Linux kernel'scaches_clean_inval_pou_macro, https://github.com/torvalds/linux/blob/551c722f40809618230001baccf219193e22fc5a/arch/arm64/mm/cache.S#L28-L43, is the same apart from usingdsb ishstwhen IDC is set):dc cvauper DminLine unless CTR_EL0.IDC,dsb ish,ic ivauper IminLine followed bydsb ishunless CTR_EL0.DIC, thenisb, with CTR_EL0 read once. compiler-rt uses this inline sequence on every aarch64 target except Apple and Windows.mrs ctr_el0at EL0 gets SIGILL on macOS, so the Apple path callssys_icache_invalidateas compiler-rt does on Darwin (https://github.com/llvm/llvm-project/blob/3390613ccf0fbbb40026dcbabb36fce444f79480/compiler-rt/lib/builtins/clear_cache.c#L225-L227); the Windows path already usesFlushInstructionCacheand is unchanged.On Graviton1 (Cortex-A72, IDC=0) the reproducer from #14442 over 200,000 iterations ran the previous function 146,886 times with 49.0.1's
clear_cacheand 0 times with this branch's final commit (a1.xlarge); the count varies between runs, 180,425 on an a1.medium with the previous revision, as in the commit message. The new hardware test fails at the first rewrite on the old code there, but it can only fail on IDC=0 cores, so the CTR_EL0 decoding is unit-tested separately; the crate's tests also pass on macOS (M4 Max), an RK3588 (A55+A76, IDC=1) and under qemu-aarch64 (cortex-a53, max).Fixes #14442