Subrata Banik has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/36542 )
Change subject: soc/intel/icelake: Make TSEG_BASE align to TSEG_SIZE ......................................................................
soc/intel/icelake: Make TSEG_BASE align to TSEG_SIZE
Change-Id: I77d1cb2fd287f45859cde37a564ea7c147d5633f Signed-off-by: Subrata Banik subrata.banik@intel.com --- M src/soc/intel/icelake/smmrelocate.c 1 file changed, 1 insertion(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/42/36542/1
diff --git a/src/soc/intel/icelake/smmrelocate.c b/src/soc/intel/icelake/smmrelocate.c index edcc49d..6ac17a2 100644 --- a/src/soc/intel/icelake/smmrelocate.c +++ b/src/soc/intel/icelake/smmrelocate.c @@ -178,6 +178,7 @@ const u32 rmask = ~(4 * KiB - 1);
smm_region(&tseg_base, &tseg_size); + tseg_base = ALIGN(tseg_base, tseg_size); smm_subregion(SMM_SUBREGION_CHIPSET, ¶ms->ied_base, ¶ms->ied_size);
/* SMRR has 32-bits of valid address aligned to 4KiB. */
Arthur Heymans has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36542 )
Change subject: soc/intel/icelake: Make TSEG_BASE align to TSEG_SIZE ......................................................................
Patch Set 1:
(1 comment)
https://review.coreboot.org/c/coreboot/+/36542/1/src/soc/intel/icelake/smmre... File src/soc/intel/icelake/smmrelocate.c:
https://review.coreboot.org/c/coreboot/+/36542/1/src/soc/intel/icelake/smmre... PS1, Line 181: tseg_base = ALIGN(tseg_base, tseg_size); I'm not sure if this is the best idea. I think you want to not program SMRR and warn about it. See cpu/intel/smm/gen1/smmrelocate.c.
Btw the cpu/intel file is used over many different targets and looks very similar to this code. It might be worth looking into making this or the gen1/ssmrelocate.c file common?
Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36542 )
Change subject: soc/intel/icelake: Make TSEG_BASE align to TSEG_SIZE ......................................................................
Patch Set 2:
(1 comment)
https://review.coreboot.org/c/coreboot/+/36542/1/src/soc/intel/icelake/smmre... File src/soc/intel/icelake/smmrelocate.c:
https://review.coreboot.org/c/coreboot/+/36542/1/src/soc/intel/icelake/smmre... PS1, Line 181: tseg_base = ALIGN(tseg_base, tseg_size);
Btw the cpu/intel file is used over many different targets and looks very similar to this code. It might be worth looking into making this or the gen1/ssmrelocate.c file common?
oh yes, someone is our team already started working on making this SMM related module common for soc/intel/common/block/smm. May be we will get rid of this in coming time soon.
Hello Patrick Rudolph, Arthur Heymans, build bot (Jenkins),
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/36542
to look at the new patch set (#3).
Change subject: soc/intel/icelake: Add alignment check for TSEG base and size ......................................................................
soc/intel/icelake: Add alignment check for TSEG base and size
This patch ensures to not set SMRR if TSEG base is not align with TSEG size
Change-Id: I77d1cb2fd287f45859cde37a564ea7c147d5633f Signed-off-by: Subrata Banik subrata.banik@intel.com --- M src/soc/intel/icelake/smmrelocate.c 1 file changed, 7 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/42/36542/3
Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36542 )
Change subject: soc/intel/icelake: Add alignment check for TSEG base and size ......................................................................
Patch Set 3:
(1 comment)
https://review.coreboot.org/c/coreboot/+/36542/1/src/soc/intel/icelake/smmre... File src/soc/intel/icelake/smmrelocate.c:
https://review.coreboot.org/c/coreboot/+/36542/1/src/soc/intel/icelake/smmre... PS1, Line 181: tseg_base = ALIGN(tseg_base, tseg_size);
[…]
Done
Arthur Heymans has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36542 )
Change subject: soc/intel/icelake: Add alignment check for TSEG base and size ......................................................................
Patch Set 3: Code-Review+2
Subrata Banik has submitted this change. ( https://review.coreboot.org/c/coreboot/+/36542 )
Change subject: soc/intel/icelake: Add alignment check for TSEG base and size ......................................................................
soc/intel/icelake: Add alignment check for TSEG base and size
This patch ensures to not set SMRR if TSEG base is not align with TSEG size
Change-Id: I77d1cb2fd287f45859cde37a564ea7c147d5633f Signed-off-by: Subrata Banik subrata.banik@intel.com Reviewed-on: https://review.coreboot.org/c/coreboot/+/36542 Reviewed-by: Arthur Heymans arthur@aheymans.xyz Tested-by: build bot (Jenkins) no-reply@coreboot.org --- M src/soc/intel/icelake/smmrelocate.c 1 file changed, 7 insertions(+), 0 deletions(-)
Approvals: build bot (Jenkins): Verified Arthur Heymans: Looks good to me, approved
diff --git a/src/soc/intel/icelake/smmrelocate.c b/src/soc/intel/icelake/smmrelocate.c index edcc49d..8f56ad6 100644 --- a/src/soc/intel/icelake/smmrelocate.c +++ b/src/soc/intel/icelake/smmrelocate.c @@ -178,6 +178,13 @@ const u32 rmask = ~(4 * KiB - 1);
smm_region(&tseg_base, &tseg_size); + + if (!IS_ALIGNED(tseg_base, tseg_size)) { + printk(BIOS_WARNING, + "TSEG base not aligned with TSEG SIZE! Not setting SMRR\n"); + return; + } + smm_subregion(SMM_SUBREGION_CHIPSET, ¶ms->ied_base, ¶ms->ied_size);
/* SMRR has 32-bits of valid address aligned to 4KiB. */
-
Arthur Heymans (Code Review) -
Subrata Banik (Code Review)