Aaron Durbin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35993 )
Change subject: cpu/x86: Add a prog_run hook to set up caching of XIP stages ......................................................................
Patch Set 8:
(5 comments)
https://review.coreboot.org/c/coreboot/+/35993/8/src/cpu/x86/mtrr/xip_cache.... File src/cpu/x86/mtrr/xip_cache.c:
https://review.coreboot.org/c/coreboot/+/35993/8/src/cpu/x86/mtrr/xip_cache.... PS8, Line 20: #define MAX_ROM_CACHE_SIZE (256 * KiB) What's the reasoning for only limiting to 256KiB? Please document your assumptions/thinking.
https://review.coreboot.org/c/coreboot/+/35993/8/src/cpu/x86/mtrr/xip_cache.... PS8, Line 22: void platform_prog_run(struct prog *prog) If this is being ran multiple times aren't we just accumulating MTRRs? Admittedly it's currently only possible to run in bootblock and verstage, but we would have accumulated 2 MTRRs then.
https://review.coreboot.org/c/coreboot/+/35993/8/src/cpu/x86/mtrr/xip_cache.... PS8, Line 33: if (cpu_info.x86 == 0xf) { I'm not sure this a valid check. x86 is already checked for 0xf in get_fms(). Can you please explain what this check is for?
https://review.coreboot.org/c/coreboot/+/35993/8/src/cpu/x86/mtrr/xip_cache.... PS8, Line 52: >> 10 Just divide by 'KiB'. The compiler will turn it in to a shift.
https://review.coreboot.org/c/coreboot/+/35993/8/src/cpu/x86/mtrr/xip_cache.... PS8, Line 58: let's make the most use out of it */ Please indicate what the logic is doing. I believe you are just trying to cover the most amount of the program.