Eugene Myers has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45181 )
Change subject: security/intel/stm: Fix size_t printf format error ......................................................................
Patch Set 1:
Patch Set 7:
(1 comment)
Patch Set 7:
(1 comment)
Patch Set 7:
(1 comment)
Patch Set 3:
Patch Set 3:
Is the STM a fully-fledged payload now? If not, it should presumably continue to be located in security/intel/stm
The STM is not by the coreboot definition a payload.
This patch is intended to replace the blob located in 3rdparty/blobs/cpu/intel/stm with source that allows the STM to be built as part of the coreboot build.
The code in security/intel/stm currently takes the stm blob and moves it to the MSEG. It also initializes some data structures, etc. so that when the STM is started, it can find the SMI handler and the BIOS resource list.
Since the STM is no longer a blob, it seems that it should not be in its current location in blobs. Earlier, it was suggested that the STM build be located in the payloads directory, but that suggestion was some time ago.
I can see a couple of options for locating the STM build aside from payloads:
(1) Somewhere under 3rdparty, but not in blobs. (2) under security/intel/stm
So, my question is: Where should the STM be located when it is built as part of coreboot?
Thanks for the explanation and background. I agree that 3rdparty/stm is a good place for the submodule.
Has this been tested yet? Build currently fails for me with: `src/security/intel/stm/SmmStm.c: In function 'add_pi_resource': src/security/intel/stm/SmmStm.c:480:21: error: format '%lx' expects argument of type 'long unsigned int', but argument 3 has type 'size_t' {aka 'unsigned int'} [-Werror=format=] 480 | printk(BIOS_DEBUG, "STM: ResourceSize - 0x%08lx\n", resource_size);`
If this wasn't intended, this can be fixed by changing the format to `%x`.
Interesting, this was building and running on my Purism Laptop and builds okay under gerrit. I checked my coreboot console output to verify. However, I will fix this as suggested.
It was with GCC 10.2.1 on Fedora. I've seen the warning about using other toolchains, so, if this is operating as expected, I can drop the issue. However, it may be due to GCC 10 as a whole. I'll test coreboot's GCC 10 (CB:42251), but if so, I can bring this up over there.
I've submitted CB:45181, which should make that printf statement compiler agnostic.