Tim Wawrzynczak has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/34797 )
Change subject: soc/intel/common/smm: Remove redundant masking op ......................................................................
soc/intel/common/smm: Remove redundant masking op
The next operation using the 'data' variable after the offending mask op is a read, indicating the masking op is not unused.
Change-Id: I71da74e5e08e7d7e6d61c1925db19324efd73f0a Found-by: Coverity CID 1381621 Signed-off-by: Tim Wawrzynczak twawrzynczak@chromium.org --- M src/soc/intel/common/block/smm/smitraphandler.c 1 file changed, 0 insertions(+), 1 deletion(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/97/34797/1
diff --git a/src/soc/intel/common/block/smm/smitraphandler.c b/src/soc/intel/common/block/smm/smitraphandler.c index bfa9846..e71e00c2 100644 --- a/src/soc/intel/common/block/smm/smitraphandler.c +++ b/src/soc/intel/common/block/smm/smitraphandler.c @@ -107,7 +107,6 @@ printk(BIOS_DEBUG, "SMI1 command\n"); /* Trapped write data */ data = pcr_read32(PID_PSTH, PCR_PSTH_TRPD); - data &= mask; } }
Jacob Garber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34797 )
Change subject: soc/intel/common/smm: Remove redundant masking op ......................................................................
Patch Set 3:
(1 comment)
https://review.coreboot.org/c/coreboot/+/34797/1/src/soc/intel/common/block/... File src/soc/intel/common/block/smm/smitraphandler.c:
https://review.coreboot.org/c/coreboot/+/34797/1/src/soc/intel/common/block/... PS1, Line 109: data = Coverity will likely complain now that this assignment isn't used, which makes me wonder why pcr_read32() is called at all. Perhaps we could printk() it in the debug message like the next time it's read?
Tim Wawrzynczak has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34797 )
Change subject: soc/intel/common/smm: Remove redundant masking op ......................................................................
Patch Set 3:
(1 comment)
https://review.coreboot.org/c/coreboot/+/34797/1/src/soc/intel/common/block/... File src/soc/intel/common/block/smm/smitraphandler.c:
https://review.coreboot.org/c/coreboot/+/34797/1/src/soc/intel/common/block/... PS1, Line 109: data =
Coverity will likely complain now that this assignment isn't used, which makes me wonder why pcr_rea […]
Hm, that's a good point. What do you think about: (void)pcr_read32(PID_PSTH, PCR_PSTH_TRPD); possibly in both places? Because pcr_read32 calls read32 which takes a volatile pointer, the compiler *shouldn't* optimize it away. The (void) makes it clear this action is done purely for its side-effects.
Jacob Garber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34797 )
Change subject: soc/intel/common/smm: Remove redundant masking op ......................................................................
Patch Set 3:
(1 comment)
https://review.coreboot.org/c/coreboot/+/34797/1/src/soc/intel/common/block/... File src/soc/intel/common/block/smm/smitraphandler.c:
https://review.coreboot.org/c/coreboot/+/34797/1/src/soc/intel/common/block/... PS1, Line 109: data =
Hm, that's a good point. What do you think about: […]
That sounds reasonable, though it would be good to get some more eyes on this after, since I don't really know what this code does.
Martin Roth has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34797 )
Change subject: soc/intel/common/smm: Remove redundant masking op ......................................................................
Patch Set 5:
(1 comment)
That whole
https://review.coreboot.org/c/coreboot/+/34797/5/src/soc/intel/common/block/... File src/soc/intel/common/block/smm/smitraphandler.c:
https://review.coreboot.org/c/coreboot/+/34797/5/src/soc/intel/common/block/... PS5, Line 109: : I agree with the earlier top level conversation. since this is in the monitor code, it seems like we're just missing a printk here and the mask of the data should be left in place.
Hello Patrick Rudolph, Subrata Banik, Paul Menzel, build bot (Jenkins), Lijian Zhao, Martin Roth, Furquan Shaikh,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/34797
to look at the new patch set (#6).
Change subject: soc/intel/common/smm: Add missing printk statement ......................................................................
soc/intel/common/smm: Add missing printk statement
SMI trap handler was missing a printk statement, which caused Coverity to flag "data &= mask;" as a redundant operation.
Change-Id: I71da74e5e08e7d7e6d61c1925db19324efd73f0a Found-by: Coverity CID 1381621 Signed-off-by: Tim Wawrzynczak twawrzynczak@chromium.org --- M src/soc/intel/common/block/smm/smitraphandler.c 1 file changed, 1 insertion(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/97/34797/6
Tim Wawrzynczak has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34797 )
Change subject: soc/intel/common/smm: Add missing printk statement ......................................................................
Patch Set 6:
(1 comment)
https://review.coreboot.org/c/coreboot/+/34797/5/src/soc/intel/common/block/... File src/soc/intel/common/block/smm/smitraphandler.c:
https://review.coreboot.org/c/coreboot/+/34797/5/src/soc/intel/common/block/... PS5, Line 109: :
I agree with the earlier top level conversation. […]
Ack
Tim Wawrzynczak has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34797 )
Change subject: soc/intel/common/smm: Add missing printk statement ......................................................................
Patch Set 6:
(1 comment)
https://review.coreboot.org/c/coreboot/+/34797/1/src/soc/intel/common/block/... File src/soc/intel/common/block/smm/smitraphandler.c:
https://review.coreboot.org/c/coreboot/+/34797/1/src/soc/intel/common/block/... PS1, Line 109: data =
That sounds reasonable, though it would be good to get some more eyes on this after, since I don't r […]
Done
Martin Roth has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34797 )
Change subject: soc/intel/common/smm: Add missing printk statement ......................................................................
Patch Set 6: Code-Review+2
Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34797 )
Change subject: soc/intel/common/smm: Add missing printk statement ......................................................................
Patch Set 6: Code-Review+2
Patrick Georgi has submitted this change and it was merged. ( https://review.coreboot.org/c/coreboot/+/34797 )
Change subject: soc/intel/common/smm: Add missing printk statement ......................................................................
soc/intel/common/smm: Add missing printk statement
SMI trap handler was missing a printk statement, which caused Coverity to flag "data &= mask;" as a redundant operation.
Change-Id: I71da74e5e08e7d7e6d61c1925db19324efd73f0a Found-by: Coverity CID 1381621 Signed-off-by: Tim Wawrzynczak twawrzynczak@chromium.org Reviewed-on: https://review.coreboot.org/c/coreboot/+/34797 Tested-by: build bot (Jenkins) no-reply@coreboot.org Reviewed-by: Martin Roth martinroth@google.com Reviewed-by: Subrata Banik subrata.banik@intel.com --- M src/soc/intel/common/block/smm/smitraphandler.c 1 file changed, 1 insertion(+), 0 deletions(-)
Approvals: build bot (Jenkins): Verified Martin Roth: Looks good to me, approved Subrata Banik: Looks good to me, approved
diff --git a/src/soc/intel/common/block/smm/smitraphandler.c b/src/soc/intel/common/block/smm/smitraphandler.c index bfa9846..974c489 100644 --- a/src/soc/intel/common/block/smm/smitraphandler.c +++ b/src/soc/intel/common/block/smm/smitraphandler.c @@ -108,6 +108,7 @@ /* Trapped write data */ data = pcr_read32(PID_PSTH, PCR_PSTH_TRPD); data &= mask; + printk(BIOS_DEBUG, " iotrap read data = 0x%08x\n", data); } }
-
Jacob Garber (Code Review) -
Martin Roth (Code Review)
-
Patrick Georgi (Code Review)
-
Subrata Banik (Code Review)
-
Tim Wawrzynczak (Code Review)