ron minnich has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33234 )
Change subject: security/intel/stm: Add STM support ......................................................................
Patch Set 6:
(6 comments)
My comments here are mostly nits. The one big item I noticed is use of typedef on struct. coreboot a long time ago moved to the linux kernel convention of avoiding typedefs for structs. I'm wondering what others think here -- let this go in and fix it later, or fix it now, or just leave it?
you can find the long argument here: https://yarchive.net/comp/linux/typedefs.html
Note that I don't necessarily agree. The folks who invented C and the Unix kernel, when they got to Plan 9 in the 1980s, used typedef structs *exactly* the way torvalds claims you should not use them. I tend to take ken and dmr's words more than I take Linus', but not everyone does.
Overall, I think this is one of the most exciting things to go into coreboot in many years. I'm looking forward to being able to use it. This code holds out the promise of providing a VM for x86 that gives guests true bare metal performance (unless I misunderstand). Thanks for your contribution and (in advance) your patience with our review process.
I do suggest you at least give clang-fmt a try and see how many of your code formatting issues it resolves.
https://review.coreboot.org/#/c/33234/6/src/security/intel/stm/Makefile.inc File src/security/intel/stm/Makefile.inc:
https://review.coreboot.org/#/c/33234/6/src/security/intel/stm/Makefile.inc@... PS6, Line 2: # put the stm where is can be found where *it*
https://review.coreboot.org/#/c/33234/6/src/security/intel/stm/Makefile.inc@... PS6, Line 4: cbfs-files-y += stm.bin the convention here would be to replace -y with $(CONFIG_STM) so I'm wondering why you did not do that? Is there some other thing I'm missing?
https://review.coreboot.org/#/c/33234/6/src/security/intel/stm/SmmStm.h File src/security/intel/stm/SmmStm.h:
https://review.coreboot.org/#/c/33234/6/src/security/intel/stm/SmmStm.h@20 PS6, Line 20: #define IA32_VMX_BASIC_MSR_INDEX 0x480 is there any reason not to just put these in src/include/cpu/x86/msr.h
https://review.coreboot.org/#/c/33234/6/src/security/intel/stm/SmmStm.c File src/security/intel/stm/SmmStm.c:
https://review.coreboot.org/#/c/33234/6/src/security/intel/stm/SmmStm.c@27 PS6, Line 27: #define RDWR_ACCS 3 It would be much better if we could get most of these constants in a generic place. They are not really STM. But if you can commit to making those changes later I'm ok with it.
https://review.coreboot.org/#/c/33234/6/src/security/intel/stm/SmmStm.c@369 PS6, Line 369: //
You usually use tabs instead of space. Also C style comments are preferred over C++. […]
if you're willing to do a clang-fmt pass and check the output that would save some work?
If you want you can get rid of the lines that have only a //. They're not that helpful IMHO.
https://review.coreboot.org/#/c/33234/6/src/security/intel/stm/SmmStm.c@548 PS6, Line 548: //mStmResourcesPtr = (uint8_t *)(UINTN)NewResource; if you have commented code there are better options, since one day it may be uncommented for some reason and won't compile. Consider if (0) {...} for code you want to leave here for some reason but not run. Or, just remove the commented code.