Aaron Durbin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36661 )
Change subject: [RFC]cpu/x86/smm: Add a unified way of handling save_states ......................................................................
Patch Set 2:
(6 comments)
https://review.coreboot.org/c/coreboot/+/36661/2/src/cpu/x86/smm/smm_save_st... File src/cpu/x86/smm/smm_save_state.c:
https://review.coreboot.org/c/coreboot/+/36661/2/src/cpu/x86/smm/smm_save_st... PS2, Line 24: void * use uint8_t * here?
https://review.coreboot.org/c/coreboot/+/36661/2/src/cpu/x86/smm/smm_save_st... PS2, Line 34: return base; I feel like this logic should be tied to the code doing the SMM memory layout. And not carrying those assumptions to another piece of code. Looks like you lifted it from smm_get_save_state() in src/cpu/x86/smm/smm_module_handler.c which is where it should belong.
https://review.coreboot.org/c/coreboot/+/36661/2/src/cpu/x86/smm/smm_save_st... PS2, Line 41: 0x8000 - 0x7efc Don't we have some defines for this?
https://review.coreboot.org/c/coreboot/+/36661/2/src/cpu/x86/smm/smm_save_st... PS2, Line 258: int smm_save_state_ops_init(const struct smm_runtime *runtime) And this would get called as one of the first things in the SMM entry point?
https://review.coreboot.org/c/coreboot/+/36661/2/src/cpu/x86/smm/smm_save_st... PS2, Line 261: smm_runtime = runtime; comment this for its purposes.
https://review.coreboot.org/c/coreboot/+/36661/2/src/include/cpu/x86/smm.h File src/include/cpu/x86/smm.h:
https://review.coreboot.org/c/coreboot/+/36661/2/src/include/cpu/x86/smm.h@1... PS2, Line 124: int smm_save_state_ops_init(const struct smm_runtime *runtime); Document the functions, please.