ron minnich has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38322 )
Change subject: Revert "security/intel/stm: Add STM support" ......................................................................
Patch Set 1:
(2 comments)
I suspect the failing code is sensitive to the inclusion of two new struct members, not the change in struct size. Consider putting the new struct members at the end.
I'm still surprised this happens, however. Does anyone know why?
https://review.coreboot.org/c/coreboot/+/38322/1/src/cpu/x86/smm/smm_stub.S File src/cpu/x86/smm/smm_stub.S:
https://review.coreboot.org/c/coreboot/+/38322/1/src/cpu/x86/smm/smm_stub.S@... PS1, Line 47: num_cpus:
These fields have to correspond with smm_runtime. […]
option 2 is to just reorder it so this field is at the END of the structure, so that you maintain compatibility with different code. I'd prefer that for all kinds of reasons anyway; best when growing a struct to just extend it. the apic_to_cpu_num is a fixed size array known at compile time.
Can we consider that? It is best that we learn now how to modify this struct, rather than having to leave it fixed for all time.
https://review.coreboot.org/c/coreboot/+/38322/1/src/include/cpu/x86/smm.h File src/include/cpu/x86/smm.h:
https://review.coreboot.org/c/coreboot/+/38322/1/src/include/cpu/x86/smm.h@a... PS1, Line 67: u32 num_cpus; were you to follow my idea, you just move these two new struct members to the end of the struct. This is no problem I suspect with the apic_id_to_cpu array, since that size is known at compile time.
If there is any way to change and test this, rather than revert, that seems preferable to me.
I'd prefer that approach to guards; it's a common way to change a struct. I'm still surprised this happened again, but ...