Hi Jeremy, On Wed, Apr 10, 2024 at 9:30 AM Jeremy Linton <jeremy.linton@xxxxxxx> wrote: > > Hi, > > > First thanks for working on this. > > On 4/7/24 07:38, Yunhui Cui wrote: > > When the type and level information of this_leaf cannot be obtained > > from arch, cacheinfo is completely filled in with the content of PPTT. > > I started reviewing this, based on what I understood to be the need to > generate the topology entirely from the PPTT. But, it was raising more > questions than answers because the PPTT is far too flexable in its > ability to represent cache hierachies that arn't logically useful. For > example multiple I or D caches at the same level, or I or D caches > higher in the topology than unified ones. > > At least for arm64 (and I think others) there is an understood > simplification that there will be N levels of split I/D caches and M > unified levels. And from that, the number of cache leaves are computed > and allocated, and then we go in and largly skip PPTT cache nodes which > don't make sense in view of a generic topology like that. (see the > comment in cacheinfo.c:506) > > Both of those pieces of information are available in > acpi_get_cache_info(). The missing part is marking those N levels of I/D > cache as such. > > Looking at this code I don't really see all the error/allocation > logic/etc that assures the cache leaf indexing is allocated correctly > which worries me, although admidditly I could be missing something > important. > > In summary, did you consider just allocating matching I/D caches from > the number of split levels in acpi_get_cache_info() then removing or > invalidating the ones that don't have matching PPTT entries after > running cache_setup_acpi()? Thats a fairly trivial change AFAIK if the > decision is based on the lack of a cache_id or just changing the > this_leaf->type = CACHE_TYPE_UNIFIED assignment to the correct type and > assuring left over CACHE_TYPE_NOCACHE entries are removed. I think much > of the "significant work" is likely fixed for that to work. Just > tweaking detect_cache_level()/get_cache_type() to set > CACHE_TYPE_SEPERATE if the level is less than the acpi_get_cache_info() > split_level value probably also does the majority of what you need > outside of having unequal counts of I and D caches. > > There are probably other choices as well, thoughts? > First, I think the current state of the ACPI PPTT specification meets the requirements and is logically complete. Otherwise, the PPTT specification needs to be updated, right? Our discussion is best focused on the existing and usual case, even on ARM64, which is as you say "N-level separated I/D cache, M-level unified". And then, the problem we have now is that the RISC-V architecture does not have a set of registers to describe the cache level and type like ARM64 does, so we need to fully trust the contents of the PPTT table. Please check the patch: https://patchwork.kernel.org/project/linux-riscv/patch/20240407123829.36474-2-cuiyunhui@xxxxxxxxxxxxx/ . Judging from this requirement, can you help review this patch? Thanks. Thanks, Yunhui
