Revert arm64 cache geometry
Get series with b4
b4 am [email protected]Versions newest first
- v1 Latest0 of 3 patches 0 repliesNew
Patches in v1 No applies recorded yet
None of the 3 patches the cover letter announces are in this archive.
Reviews trailers per patch
No Reviewed-by, Acked-by or Tested-by trailers for v1 yet.
Cover letter [PATCH 0/3] Revert arm64 cache geometry
Hi Alex,
Thanks for describing the problem in depth here. I'm not at all happy about the conclusion, but you're right and thanks for the report. Minor comments below.
Quoted 29 lines · Alex Van Brunt
This patchset reverts three patches that attempt to query the CPU for cache geometry and then make use of that information. Those patches rely on the NumSets and LineSize fields of CCSIDR to determine the cache geometry. However, the architectural documentation for these registers forbids such use:
The parameters NumSets, Associativity, and LineSize in these registers define the architecturally visible parameters that are required for the cache maintenance by Set/Way instructions. They are not guaranteed to represent the actual microarchitectural features of a design. You cannot make any inference about the actual sizes of caches based on these parameters.
It is not just theoretical. For example, the Denver CPU will report one set and one way in CCSIDR even though the actual microarchitectural implementation has many sets and many ways.
I have two suggestions for how to get the cache geometry on an ARMv8 processor:
1. Specify the information in the device tree. The purpose of the deivce tree is to specify information that software cannot query at run-time. Becuase the architecture does not have an architectural way to query the cache geometry this may be a good fit.
2. Add a function pointer tocpu_tablethat gives a implementation specific way to query the cache geometry. For an A57, for example, the function could read the CCSIDR register because it happens to report the microarchitectural geometry. The Denver CPU has implementation defined registers that can be used to determine the microarchitectural geometry. However, the implementation for the default "AArch64 Processor", must return an error.
Quoted 9 lines
The only place that the cache geometry is used is to determine if there can be aliasing for a VIPT (virtually-indexed, physically-tagged) instruction cache. The code assumes that there is no need to flush the entire instruction cache if the size of a cache set is less than or equal to a page size. However, the architectural definition of VIPT says "The only architecturally-guaranteed way to invalidate all aliases of a physical address from a VIPT instruction cache is to invalidate the entire instruction cache." Not only are the parameters not guaranteed to be correct, it is explicitly not legal to ignore aliasing even if the parameters were correct.
This is useful detail -- can you include it in the relevant commit messages too, please? You can also drop the ChangeId tags and probably add Cc:
<[email protected]> tags as well.
We should also add a comment to the I-cache aliasing code to state why we always nuke the entire I-cache, so that we don't "optimise" it again in the future!
My final concern is how this impacts userspace parsing /sys/devices/system/cpu/cpuX/cache/*. Do we need to stub that out with dummy values and extend the device-tree properties to allow inner-cache geometry to be described? I worry that simply removing the files under there could break more than it solves.
Perhaps the right solution is to leave the cacheinfo code as-is and extend it so that it prefers to use DT, falling back to the registers if the properties are absent? That matches up with our treatment of MPIDR for topology too and reduces the risk of breaking any existing software.
Patch 2 and 3 look fine as straight reverts, though (module my previous comments about commit messages).
Will