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.
6 comments:
File src/security/intel/stm/Makefile.inc:
Patch Set #6, Line 2: # put the stm where is can be found
where *it*
Patch Set #6, 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?
File src/security/intel/stm/SmmStm.h:
Patch Set #6, 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
File src/security/intel/stm/SmmStm.c:
Patch Set #6, 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.
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.
Patch Set #6, 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.
To view, visit change 33234. To unsubscribe, or for help writing mail filters, visit settings.