Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36382 )
Change subject: cpu/x86/mtrr: Add function to set MTRR with CR0.CD set ......................................................................
Patch Set 1:
(6 comments)
https://review.coreboot.org/c/coreboot/+/36382/1/src/cpu/x86/mtrr/earlymtrr.... File src/cpu/x86/mtrr/earlymtrr.c:
https://review.coreboot.org/c/coreboot/+/36382/1/src/cpu/x86/mtrr/earlymtrr.... PS1, Line 55: Please comment on the purpose of the function.
https://review.coreboot.org/c/coreboot/+/36382/1/src/cpu/x86/mtrr/earlymtrr.... PS1, Line 56: uncached Not convinced by the name, but I guess it doesn't matter that much if you add a comment. set_var_mtrr_during_car()?
https://review.coreboot.org/c/coreboot/+/36382/1/src/cpu/x86/mtrr/earlymtrr.... PS1, Line 60: Bit s/Bit Bit/Bits/ ;)
https://review.coreboot.org/c/coreboot/+/36382/1/src/cpu/x86/mtrr/earlymtrr.... PS1, Line 61: support support*s*
https://review.coreboot.org/c/coreboot/+/36382/1/src/cpu/x86/mtrr/set_mtrr.S File src/cpu/x86/mtrr/set_mtrr.S:
https://review.coreboot.org/c/coreboot/+/36382/1/src/cpu/x86/mtrr/set_mtrr.S... PS1, Line 29: pushl %ebx : pushl %edx : pushl %esi : pushl %edi : : movl 0(%ebp), %ecx : movl 4(%ebp), %ebx : movl 8(%ebp), %edx : movl 12(%ebp), %esi : movl 16(%ebp), %edi Lots of boilerplate to ensure that we don't access memory while CD is set. It's worth a comment that this is a requirement, IMHO.
https://review.coreboot.org/c/coreboot/+/36382/1/src/cpu/x86/mtrr/set_mtrr.S... PS1, Line 56: /* Enable cache again. */ What if it wasn't enabled before? Sure, we shouldn't have called it. That's why I think we should make more clear when it should be used.