Aaron Durbin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37196 )
Change subject: cpu/x86/cache: CLFLUSH programs to memory before running ......................................................................
Patch Set 7:
(3 comments)
https://review.coreboot.org/c/coreboot/+/37196/7/src/cpu/x86/cache/cache.c File src/cpu/x86/cache/cache.c:
https://review.coreboot.org/c/coreboot/+/37196/7/src/cpu/x86/cache/cache.c@2... PS7, Line 27: int clflush_supported(void)
This is not sufficient. When CAR is active it's microarchitecturally defined how a clflush is handled in CAR mode. You need a way for the chipset to indicate this is valid sequence that can be performed.
This seems to work for Intel CPUs with a NEM and also older ones lacking it. Subrata tested a similar concept with WB caching cbmem on much more recent Intel CPU's (probably NEM enhanced). So at first sight it looks like this will be ok for most Intel? Probably worth checking for some AMD.
Those results are clearly empirical which suggests that the chipsets need to opt-in to affirm this will work.
https://review.coreboot.org/c/coreboot/+/37196/7/src/cpu/x86/cache/cache.c@2... PS7, Line 29: 19
Aren't there defines for these bits instead of open coding them? […]
Can you start?
https://review.coreboot.org/c/coreboot/+/37196/7/src/cpu/x86/cache/cache.c@6... PS7, Line 60: if (!cbmem_ready())
Why is a cbmem check in here? It doesn't immediately make sense why this would be the case. Likewise, this function is also active when not loading postcar, e.g. FSP. What about that? Or are you using cbmem to be an indirect proxy for postcar loading?
It wanted to use that as proxy that dram is ready and avoid it when things are loaded into CAR like FSP (if non-XIP). Is checking for [start, start + size] being inside the CAR region sufficient/better?
If that's the case then please comment the purpose of what the check is for. It's clearly indirect w.r.t. to what you want accomplish from your explanation.