Arthur Heymans 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:
(4 comments)
https://review.coreboot.org/c/coreboot/+/35993/5//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/35993/5//COMMIT_MSG@16 PS5, Line 16: fine on at least model_1067x and model_6ex.
I'm not an expert, but would feel safer with CD set. Unless there is some implicit invalidation ofc.
There is not from my experiments doing it in assembly. I don't think CAR is accessible with CD set (not tested), so an assembly version copying all the function parameters from stack to registers is required if one wants to do this. CB:35722 does more or less that but it needs to be written more cleanly though.
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 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.
That's correct and tested to work fine.
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?
family 0xf, pentium 4, has CAR issues when ROM is cached, this is also done in cpu/intel/car/p4/cache_as_ram.S:
/* * An unidentified combination of speculative reads and branch * predictions inside WRPROT-cacheable memory can cause invalidation * of cachelines and loss of stack on models based on NetBurst * microarchitecture. Therefore disable WRPROT region entirely for * all family F models. */
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.
That's correct.