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 9:
(7 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.
I recently tried CAR with CR0.CD set instead of unset. With the rom cached, no matter what type, it caused problems for CAR, with only a CAR region set up not. I'm not so sure if setting CR0.CD is a good idea here but I can try.
https://review.coreboot.org/c/coreboot/+/35993/6/src/cpu/x86/mtrr/xip_cache.... File src/cpu/x86/mtrr/xip_cache.c:
https://review.coreboot.org/c/coreboot/+/35993/6/src/cpu/x86/mtrr/xip_cache.... PS6, Line 59: mtrr_base = ALIGN_UP(base, mtrr_mask_size);
Still, I think we need to override the ALIGN_DOWN() above. […]
Done
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.
Done
https://review.coreboot.org/c/coreboot/+/35993/8/src/cpu/x86/mtrr/xip_cache.... PS8, Line 22: void platform_prog_run(struct prog *prog)
Working fine is a function of the ROM layout and cpu cache microarchitecture. […]
Done
https://review.coreboot.org/c/coreboot/+/35993/8/src/cpu/x86/mtrr/xip_cache.... PS8, Line 33: if (cpu_info.x86 == 0xf) {
My point is that the x86 field is manipulated when it's == 0xf. Are you suggesting that after the manipulation the family will remain 0xf on netburst?
Ack. Netburst is exactly 0xf even with addition of the extended family bits (0).
Either way, there needs to be a comment added describing what you are trying to accomplish.
ack.
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.
Done
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. […]
Done