MIPS: c-r4k.c: Fix the 74K D-cache alias erratum workaround

Fix the 74K D-cache alias erratum workaround so that it actually works. Our current code sets MIPS_CACHE_VTAG for the D-cache, but that flag only has any effect for the I-cache. Additionally MIPS_CACHE_PINDEX is set for the D-cache if CP0.Config7.AR is also set for an affected processor, leading to confusing information in the bootstrap log (the flag isn't used beyond that).

So delete the setting of MIPS_CACHE_VTAG and rely on MIPS_CACHE_ALIASES, set in a common place, removing I-cache coherency issues seen in GDB testing with software breakpoints, gdbserver and ptrace(2), on affected systems.

While at it add a little piece of explanation of what CP0.Config6.SYND is so that people do not have to chase documentation.

Quoted 7 lines

Fix the 74K D-cache alias erratum workaround so that it actually works. Our current code sets MIPS_CACHE_VTAG for the D-cache, but that flag only has any effect for the I-cache. Additionally MIPS_CACHE_PINDEX is set for the D-cache if CP0.Config7.AR is also set for an affected processor, leading to confusing information in the bootstrap log (the flag isn't used beyond that).

Quoted 7 lines

So delete the setting of MIPS_CACHE_VTAG and rely on MIPS_CACHE_ALIASES, set in a common place, removing I-cache coherency issues seen in GDB testing with software breakpoints, gdbserver and ptrace(2), on affected systems.

While at it add a little piece of explanation of what CP0.Config6.SYND is so that people do not have to chase documentation.

This shift to MIPS_CACHE_ALIASES is not needed, a use of MIPS_CACHE_VTAG in dcache is actually a way how to prevent some very specific situations in 74K(E77)/1074K(E17) cache handling. It is not a case of cache aliasing and name VTAG is used because it is related with virtual address conversion tagging. I reused MIPS_CACHE_VTAG just to save some spare bits in cpu_info.options and because D-cache never had virtual tagging like I-cache.

The setting d-cache aliases then CPU hasn't it is a significant performance loss and should be avoided.

Please don't use this patch.

Quoted 22 lines · Leonid Yegoshin

Quoted 4 lines

Fix the 74K D-cache alias erratum workaround so that it actually works. Our current code sets MIPS_CACHE_VTAG for the D-cache, but that flag only has any effect for the I-cache. Additionally MIPS_CACHE_PINDEX is set for the D-cache if CP0.Config7.AR is also set for an affected

Quoted 8 lines

processor, leading to confusing information in the bootstrap log (the flag isn't used beyond that).
So delete the setting of MIPS_CACHE_VTAG and rely on MIPS_CACHE_ALIASES, set in a common place, removing I-cache coherency issues seen in GDB testing with software breakpoints, gdbserver and ptrace(2), on affected systems.
While at it add a little piece of explanation of what CP0.Config6.SYND is so that people do not have to chase documentation.

This shift to MIPS_CACHE_ALIASES is not needed, a use of MIPS_CACHE_VTAG in dcache is actually a way how to prevent some very specific situations in 74K(E77)/1074K(E17) cache handling. It is not a case of cache aliasing and name VTAG is used because it is related with virtual address conversion tagging. I reused MIPS_CACHE_VTAG just to save some spare bits in cpu_info.options and because D-cache never had virtual tagging like I-cache.

The setting d-cache aliases then CPU hasn't it is a significant performance loss and should be avoided.

Quoted 1 line

Please don't use this patch.

I repeat, there is no use in the current kernel of the MIPS_CACHE_VTAG
flag for the D-cache, there's no single piece of code throughout the kernel that would check the presence of this flag once it has been set and this erratum workaround piece is the only place where the flag is set for the D-cache. The flag is completely ignored for the D-cache and the only existing uses of this flag are for the I-cache.

Leonid, I spent several hours chasing cache coherency issues util I
realised the workaround in its current form does not do anything, so unless you propose an alternative fix, this change is the only way known to bring systems affected to sanity.

Ralf, please apply the change for now, if Leonid provides us with a
better fix later on, then my patch can be reverted. Thanks.

Maciej

Quoted 6 lines

I repeat, there is no use in the current kernel of the MIPS_CACHE_VTAG
flag for the D-cache, there's no single piece of code throughout the kernel that would check the presence of this flag once it has been set and this erratum workaround piece is the only place where the flag is set for the D-cache. The flag is completely ignored for the D-cache and the only existing uses of this flag are for the I-cache.

Please look into http://patchwork.linux-mips.org/patch/8459/
Look into cpu_has_vtag_dcache usage.
It is a second or 2rd (or 3rd etc) attempt to put using of that information upstream for last 1.5 year.

Quoted 4 lines

Leonid, I spent several hours chasing cache coherency issues util I
realised the workaround in its current form does not do anything, so unless you propose an alternative fix, this change is the only way known to bring systems affected to sanity.

You may ask me, I work last 3 years in MIPS (now IMG) on it and did a most of coherency fixes all that time.

Quoted 10 lines · Leonid Yegoshin

Quoted 6 lines

I repeat, there is no use in the current kernel of the MIPS_CACHE_VTAG
flag for the D-cache, there's no single piece of code throughout the kernel that would check the presence of this flag once it has been set and this erratum workaround piece is the only place where the flag is set for the D-cache. The flag is completely ignored for the D-cache and the only existing uses of this flag are for the I-cache.

Please look into http://patchwork.linux-mips.org/patch/8459/
Look into cpu_has_vtag_dcache usage.
It is a second or 2rd (or 3rd etc) attempt to put using of that information upstream for last 1.5 year.

That doesn't appear to have anything to do with ptrace(2) and
cache-coherency issues seen around software breakpoints, and the buggy 74K is the only system of all that we have that shows that problem. And the problem goes away with my fix. So perhaps MIPS_CACHE_ALIASES is actually needed here, or maybe a more lightweight alternative fix, but an item that addresses HIGHMEM support is clearly irrelevant.

This is easily reproduced by running the GDB test suite, I'd have to
double-check old logs from before my fix, but it may be more prominent with MIPS16 code for some reason. So I suggest that you run the GDB test suite, for the MIPS16 multilib and on the upstream kernel as it is, on a good and a bad 74K processor and compare results, and then see if any of your outstanding fixes makes any changes.

The symptoms are either spurious SIGTRAP signals delivered (if a stale
breakpoint remaining in the I-cache was hit) or breakpoint misses (if a stale original instruction remaining in the I-cache was executed). Please note that these failures are not extensive, there'll be a couple across the whole test suite run (several thousands tests) -- it's not like software breakpoints are unusable.

Quoted 6 lines

Quoted 4 lines

Leonid, I spent several hours chasing cache coherency issues util I
realised the workaround in its current form does not do anything, so unless you propose an alternative fix, this change is the only way known to bring systems affected to sanity.

You may ask me, I work last 3 years in MIPS (now IMG) on it and did a most of coherency fixes all that time.

Well, I-cache coherency issues around ptrace(2), specifically the
PTRACE_POKETEXT request, appear to persist -- how often do you verify your code by getting the GDB test suite run on it (e.g. by your toolchain team)? Which so far is I believe the best way to trigger any I-cache coherency issues there, and I remember still seeing issues with software breakpoints in heavily threaded code across all processors I could check with (and GDB's Remote Serial Protocol logs indicated they were correctly propagated down to ptrace(2), so it was all up to the kernel to sort out), at least a 24Kc, a 74Kc and an M14Kc.

Please note that I test using gdbserver with GDB connecting remotely
from another system, I don't normally run native testing on these boards, they are too weak for that. It is an important detail as native testing implies much higher cache pressure (gdbserver is a lightweight agent while GDB itself is a large blob) where incoherent cache lines are much more likely to cure by themselves in the course of line replacement on cache fills.

Maciej

Quoted 6 lines

That doesn't appear to have anything to do with ptrace(2) and
cache-coherency issues seen around software breakpoints, and the buggy 74K is the only system of all that we have that shows that problem. And the problem goes away with my fix. So perhaps MIPS_CACHE_ALIASES is actually needed here, or maybe a more lightweight alternative fix, but an item that addresses HIGHMEM support is clearly irrelevant.

Again, the 74K errata has deal only if there is two mappings and one of that mappings switches to the same physaddr. In other words - TLB change is needed (some another conditions are also needed). It has nothing with generic cache aliasing. Switching ON cache aliasing support just masks the issue behind massive cache flushes.

If you have problem with ptrace(2) then it points to incorrect result of copy_to_user_page(), and most probably - with kmap() work. That HIGHMEM patch there takes care about address aliasings, so I assumed I took that case into account too but something may changes. I think it has sense to put the check of cpu_has_vtag_dcache in copy_to_user_page() - it definitely will enforce cache flashing after ptrace() write aka __access_remote_vm(..., true) and it doesn't harm the rest of system. And retest.

But it is needed to do after http://patchwork.linux-mips.org/patch/8459/ fix, without it it is futile.

Quoted 10 lines

Well, I-cache coherency issues around ptrace(2), specifically the
PTRACE_POKETEXT request, appear to persist -- how often do you verify your code by getting the GDB test suite run on it (e.g. by your toolchain team)? Which so far is I believe the best way to trigger any I-cache coherency issues there, and I remember still seeing issues with software breakpoints in heavily threaded code across all processors I could check with (and GDB's Remote Serial Protocol logs indicated they were correctly propagated down to ptrace(2), so it was all up to the kernel to sort out), at least a 24Kc, a 74Kc and an M14Kc.

Quoted 7 lines

Please note that I test using gdbserver with GDB connecting remotely
from another system, I don't normally run native testing on these boards, they are too weak for that. It is an important detail as native testing implies much higher cache pressure (gdbserver is a lightweight agent while GDB itself is a large blob) where incoherent cache lines are much more likely to cure by themselves in the course of line replacement on cache fills.

Excuse me, I need more detailed instructions "how to", I am not GDB guru.

Quoted 6 lines · Leonid Yegoshin

Quoted 6 lines

That doesn't appear to have anything to do with ptrace(2) and
cache-coherency issues seen around software breakpoints, and the buggy 74K is the only system of all that we have that shows that problem. And the problem goes away with my fix. So perhaps MIPS_CACHE_ALIASES is actually needed here, or maybe a more lightweight alternative fix, but an item that addresses HIGHMEM support is clearly irrelevant.

Quoted 5 lines

Again, the 74K errata has deal only if there is two mappings and one of that mappings switches to the same physaddr. In other words - TLB change is needed (some another conditions are also needed). It has nothing with generic cache aliasing. Switching ON cache aliasing support just masks the issue behind massive cache flushes.

For the erratum to trigger do the two mappings absolutely have to go
through the TLB or can one of them be via the TLB and the other one via a fixed mapping by the means of KSEG0?

This will be the case here.

And I understand what the consequence of setting MIPS_CACHE_ALIASES is,
but I also insist that correctness is more important than performance, so we need to make sure that the kernel performs reliably even if that comes at a cost. And the cost may be higher than necessary at the beginning, but that will be the right starting point for further improvements.

Quoted 8 lines

If you have problem with ptrace(2) then it points to incorrect result of copy_to_user_page(), and most probably - with kmap() work. That HIGHMEM patch there takes care about address aliasings, so I assumed I took that case into account too but something may changes. I think it has sense to put the check of cpu_has_vtag_dcache in copy_to_user_page() - it definitely will enforce cache flashing after ptrace() write aka __access_remote_vm(..., true) and it doesn't harm the rest of system. And retest.

Quoted 2 lines

But it is needed to do after http://patchwork.linux-mips.org/patch/8459/ fix, without it it is futile.

I think burying fixes for ordinary accesses among HIGHMEM pieces does
not really help, would you be able to split off pieces relevant for non-HIGHMEM configurations, such as those for the 74K workaround, from your big change? I think it would make it easier to get this part accepted, and the remaining pieces would then shrink too, also making them easier to review and accept.