Attention is currently required from: Subrata Banik, Tim Wawrzynczak, Nick Vaccaro, Eric Lai. Arthur Heymans has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/63492 )
Change subject: cpu/x86/mtrrlib: Use `need_restore_mtrr()` from set_var_mtrr function ......................................................................
Patch Set 1:
(1 comment)
File src/cpu/x86/mtrr/mtrrlib.c:
https://review.coreboot.org/c/coreboot/+/63492/comment/2db6890f_342fdc75 PS1, Line 52: /* Need to restore mtrr later using remove_temp_solution. */ : if (ENV_RAMSTAGE) : need_restore_mtrr();
I don't like this. set_var_mtrr should do just one thing: set a variable mtrr... I if you want a temp mtrr with this function, it should be the caller setting this up.
Are you suggesting to make need_restore_mtrr() from the caller of `set_var_mtrr()` ?
Yes.
I thought, after DRAM MTRR snapshot being implemented, if user called into `set_var_mtrr()` then set_var_mtrr() implement inherently sets put_back_original_solution variable. but I do see a value in your suggestion. I think try to call this call exclusive of set_var_mtrr().
What about a function that makes this temporary nature more explicit and calls both set_var_mtrr and calls need_restore_mtrr?
Hmm it looks like I assumed that mtrr_use_temp_range() was used but in fact set_var_mtrr() is used in fast_spi.c. Given that the API's of mtrr_use_temp_range and set_var_mtrr are very similar it might be a good idea to have the if (ENV_RAMSTAGE) code in soc/intel/fast_spi.c instead to use mtrr_use_temp_range? That would allow to avoid using the more manual set_var_mtrr + there would be no need to expose need_restore_mtrr.