Hello Eugene Myers,
I'd like you to do a code review. Please visit
https://review.coreboot.org/c/coreboot/+/45181
to review the following change.
Change subject: security/intel/stm: Fix size_t printf format error ......................................................................
security/intel/stm: Fix size_t printf format error
Size_t seems to have a compiler dependency. When building on the Purism librem 15v4, size_t is 'unsigned long'. In this instance, the compiler is the coreboot configured cross-compiler. In another instance, size_t is defined as 'unsigned short'. To get around the formatting conflict caused by this, The variable of type size_t was cast as 'unsigned int' in the format.
Change-Id: Id51730c883d8fb9e87183121deb49f5fdda0114e Signed-off-by: Eugene D Myers cedarhouse@comcast.net --- M src/security/intel/stm/SmmStm.c 1 file changed, 1 insertion(+), 1 deletion(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/81/45181/1
diff --git a/src/security/intel/stm/SmmStm.c b/src/security/intel/stm/SmmStm.c index 7256401..00490cf 100644 --- a/src/security/intel/stm/SmmStm.c +++ b/src/security/intel/stm/SmmStm.c @@ -477,7 +477,7 @@ return -1; // INVALID_PARAMETER;
resource_size = get_resource_size(resource_list, num_entries); - printk(BIOS_DEBUG, "STM: ResourceSize - 0x%08lx\n", resource_size); + printk(BIOS_DEBUG, "STM: ResourceSize - 0x%08x\n", (int) resource_size); if (resource_size == 0) return -1; // INVALID_PARAMETER;
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.
ron minnich 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: Code-Review+2
Patrick Georgi has submitted this change. ( https://review.coreboot.org/c/coreboot/+/45181 )
Change subject: security/intel/stm: Fix size_t printf format error ......................................................................
security/intel/stm: Fix size_t printf format error
Size_t seems to have a compiler dependency. When building on the Purism librem 15v4, size_t is 'unsigned long'. In this instance, the compiler is the coreboot configured cross-compiler. In another instance, size_t is defined as 'unsigned short'. To get around the formatting conflict caused by this, The variable of type size_t was cast as 'unsigned int' in the format.
Change-Id: Id51730c883d8fb9e87183121deb49f5fdda0114e Signed-off-by: Eugene D Myers cedarhouse@comcast.net Reviewed-on: https://review.coreboot.org/c/coreboot/+/45181 Tested-by: build bot (Jenkins) no-reply@coreboot.org Reviewed-by: ron minnich rminnich@gmail.com --- M src/security/intel/stm/SmmStm.c 1 file changed, 1 insertion(+), 1 deletion(-)
Approvals: build bot (Jenkins): Verified ron minnich: Looks good to me, approved
diff --git a/src/security/intel/stm/SmmStm.c b/src/security/intel/stm/SmmStm.c index 7256401..00490cf 100644 --- a/src/security/intel/stm/SmmStm.c +++ b/src/security/intel/stm/SmmStm.c @@ -477,7 +477,7 @@ return -1; // INVALID_PARAMETER;
resource_size = get_resource_size(resource_list, num_entries); - printk(BIOS_DEBUG, "STM: ResourceSize - 0x%08lx\n", resource_size); + printk(BIOS_DEBUG, "STM: ResourceSize - 0x%08x\n", (int) resource_size); if (resource_size == 0) return -1; // INVALID_PARAMETER;
Felix Held 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 3:
(3 comments)
https://review.coreboot.org/c/coreboot/+/45181/3//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/45181/3//COMMIT_MSG@14 PS3, Line 14: unsigned int in the code it is cast to a signed integer
https://review.coreboot.org/c/coreboot/+/45181/3/src/security/intel/stm/SmmS... File src/security/intel/stm/SmmStm.c:
https://review.coreboot.org/c/coreboot/+/45181/3/src/security/intel/stm/SmmS... PS3, Line 480: lx this should be zx
https://review.coreboot.org/c/coreboot/+/45181/3/src/security/intel/stm/SmmS... PS3, Line 480: int this is a signed type, not an unsigned like size_t is
Paul Menzel 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 3:
I though, Elyes would comment, but I think this fix is incorrect, and also has nothing to do with the platform but the build environment.
There is a dedicated length modifier for `size_t`. From `man 3 printf`:
z A following integer conversion corresponds to a size_t or ssize_t argument, or a following n conversion corresponds to a pointer to a size_t argument.
If I am right, please revert, or fix it correctly.
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 3:
(3 comments)
Patch Set 3:
I though, Elyes would comment, but I think this fix is incorrect, and also has nothing to do with the platform but the build environment.
There is a dedicated length modifier for `size_t`. From `man 3 printf`:
z A following integer conversion corresponds to a size_t or ssize_t argument, or a following n conversion corresponds to a pointer to a size_t argument.
If I am right, please revert, or fix it correctly.
This patch does fix a build environment issue and I will submit a replacement patch that correctly addresses the issue.
https://review.coreboot.org/c/coreboot/+/45181/3//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/45181/3//COMMIT_MSG@14 PS3, Line 14: unsigned int
in the code it is cast to a signed integer
Thanks for the catch, I will fix this in the upcoming update.
https://review.coreboot.org/c/coreboot/+/45181/3/src/security/intel/stm/SmmS... File src/security/intel/stm/SmmStm.c:
https://review.coreboot.org/c/coreboot/+/45181/3/src/security/intel/stm/SmmS... PS3, Line 480: lx
this should be zx
Thanks for showing me this. I will use it in the upcoming replacement patch
https://review.coreboot.org/c/coreboot/+/45181/3/src/security/intel/stm/SmmS... PS3, Line 480: int
this is a signed type, not an unsigned like size_t is
Cast will be removed in the replacement patch.
Felix Held 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 3:
i pushed a fix: https://review.coreboot.org/c/coreboot/+/45872