Patrick Rudolph has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/30932
Change subject: soc/intel/fsp_broadwell_de: Fix TSEG size computation ......................................................................
soc/intel/fsp_broadwell_de: Fix TSEG size computation
The address bits 19:0 of TSEG_LIMIT read as zero, but are ignored on comparison. The result is that the limit is effectively FFFFFh.
Add one MiB to the register value to make TSEG 8MiB instead of 7MiB. Fixes a crash related to SMRR not matching the TSEG region.
Change-Id: I1a625f7bb53a3e90d3cbc0ce16021892861367d8 Signed-off-by: Patrick Rudolph patrick.rudolph@9elements.com --- M src/soc/intel/fsp_broadwell_de/smmrelocate.c 1 file changed, 5 insertions(+), 1 deletion(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/32/30932/1
diff --git a/src/soc/intel/fsp_broadwell_de/smmrelocate.c b/src/soc/intel/fsp_broadwell_de/smmrelocate.c index f8f98c2..a58744e 100644 --- a/src/soc/intel/fsp_broadwell_de/smmrelocate.c +++ b/src/soc/intel/fsp_broadwell_de/smmrelocate.c @@ -220,9 +220,13 @@ * encompasses the SMRAM range as well as the IED range. * However, the SMRAM available to the handler is 4MiB since the IEDRAM * lives TSEG_BASE + 4MiB. + * + * Note that address bits 19:0 are ignored and not compared. + * The result is that BASE[19:0] is effectively 00000h and LIMIT is + * effectively FFFFFh. */ tseg_base = northbridge_get_base_reg(dev, TSEG_BASE); - tseg_limit = northbridge_get_base_reg(dev, TSEG_LIMIT); + tseg_limit = northbridge_get_base_reg(dev, TSEG_LIMIT) + 1 * MiB; tseg_size = tseg_limit - tseg_base;
params->smram_base = tseg_base;
Arthur Heymans has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/30932 )
Change subject: soc/intel/fsp_broadwell_de: Fix TSEG size computation ......................................................................
Patch Set 1:
(3 comments)
https://review.coreboot.org/#/c/30932/1//COMMIT_MSG Commit Message:
https://review.coreboot.org/#/c/30932/1//COMMIT_MSG@12 PS1, Line 12: Add one MiB to the register value to make TSEG 8MiB instead of 7MiB. : Fixes a crash related to SMRR not matching the TSEG region. this simply looks like a wrong size was programmed for TSEG size.
https://review.coreboot.org/#/c/30932/1/src/soc/intel/fsp_broadwell_de/smmre... File src/soc/intel/fsp_broadwell_de/smmrelocate.c:
https://review.coreboot.org/#/c/30932/1/src/soc/intel/fsp_broadwell_de/smmre... PS1, Line 229: + 1 * MiB; are you sure?
https://review.coreboot.org/#/c/30932/1/src/soc/intel/fsp_broadwell_de/smmre... PS1, Line 241: params->smrr_base.lo = (params->smram_base & rmask) | MTRR_TYPE_WRBACK; : params->smrr_base.hi = 0; : params->smrr_mask.lo = (~(tseg_size - 1) & rmask) | : MTRR_PHYS_MASK_VALID; : params->smrr_mask.hi = 0; from my experience it is likely that writing to SMRR hangs the system because base is not aligned to size or that size is not a power of 2.
Arthur Heymans has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/30932 )
Change subject: soc/intel/fsp_broadwell_de: Fix TSEG size computation ......................................................................
Patch Set 1: Code-Review+1
(1 comment)
https://review.coreboot.org/#/c/30932/1//COMMIT_MSG Commit Message:
https://review.coreboot.org/#/c/30932/1//COMMIT_MSG@12 PS1, Line 12: Add one MiB to the register value to make TSEG 8MiB instead of 7MiB. : Fixes a crash related to SMRR not matching the TSEG region.
this simply looks like a wrong size was programmed for TSEG size.
ehm nvm...
Patrick Rudolph has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/30932 )
Change subject: soc/intel/fsp_broadwell_de: Fix TSEG size computation ......................................................................
Patch Set 1:
(1 comment)
https://review.coreboot.org/#/c/30932/1/src/soc/intel/fsp_broadwell_de/smmre... File src/soc/intel/fsp_broadwell_de/smmrelocate.c:
https://review.coreboot.org/#/c/30932/1/src/soc/intel/fsp_broadwell_de/smmre... PS1, Line 229: + 1 * MiB;
are you sure?
It's explained in the commit message and the comment above. Without this patch tseg_size computes to 7MiB, but is actually 8MiB.
Arthur Heymans has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/30932 )
Change subject: soc/intel/fsp_broadwell_de: Fix TSEG size computation ......................................................................
Patch Set 1: Code-Review+2
(1 comment)
https://review.coreboot.org/#/c/30932/1/src/soc/intel/fsp_broadwell_de/smmre... File src/soc/intel/fsp_broadwell_de/smmrelocate.c:
https://review.coreboot.org/#/c/30932/1/src/soc/intel/fsp_broadwell_de/smmre... PS1, Line 229: + 1 * MiB;
It's explained in the commit message and the comment above. […]
ok. it still might be a good check for the SMRR alignment requirements before setting those parameters in general, but that is besides the scope of this patch.
Werner Zeh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/30932 )
Change subject: soc/intel/fsp_broadwell_de: Fix TSEG size computation ......................................................................
Patch Set 1: Code-Review+2
Indded, this was missing before. Thanks for fixing.
Werner Zeh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/30932 )
Change subject: soc/intel/fsp_broadwell_de: Fix TSEG size computation ......................................................................
Patch Set 1: Code-Review+1
(1 comment)
https://review.coreboot.org/#/c/30932/1/src/soc/intel/fsp_broadwell_de/smmre... File src/soc/intel/fsp_broadwell_de/smmrelocate.c:
https://review.coreboot.org/#/c/30932/1/src/soc/intel/fsp_broadwell_de/smmre... PS1, Line 229: + 1 * MiB;
ok. […]
Wait a moment...wouldn't that mean that this will end up in an "off-by-one" error? Since the lower bits for limit are 0xfffff and 1 Meg is 0x100000. Or do i oversee something?
Patrick Rudolph has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/30932 )
Change subject: soc/intel/fsp_broadwell_de: Fix TSEG size computation ......................................................................
Patch Set 1:
(2 comments)
https://review.coreboot.org/#/c/30932/1/src/soc/intel/fsp_broadwell_de/smmre... File src/soc/intel/fsp_broadwell_de/smmrelocate.c:
https://review.coreboot.org/#/c/30932/1/src/soc/intel/fsp_broadwell_de/smmre... PS1, Line 229: + 1 * MiB;
Wait a moment... […]
As the lower address bits are don't care, you decode all addresses in range [0x00000-0xfffff], which are 1 Meg in total.
https://review.coreboot.org/#/c/30932/1/src/soc/intel/fsp_broadwell_de/smmre... PS1, Line 241: params->smrr_base.lo = (params->smram_base & rmask) | MTRR_TYPE_WRBACK; : params->smrr_base.hi = 0; : params->smrr_mask.lo = (~(tseg_size - 1) & rmask) | : MTRR_PHYS_MASK_VALID; : params->smrr_mask.hi = 0;
from my experience it is likely that writing to SMRR hangs the system because base is not aligned to […]
The hang I experience is at a later point. I need to check the BWG, but it looks like 1 MiB alignment works just fine.
Werner Zeh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/30932 )
Change subject: soc/intel/fsp_broadwell_de: Fix TSEG size computation ......................................................................
Patch Set 1: Code-Review+2
(1 comment)
https://review.coreboot.org/#/c/30932/1/src/soc/intel/fsp_broadwell_de/smmre... File src/soc/intel/fsp_broadwell_de/smmrelocate.c:
https://review.coreboot.org/#/c/30932/1/src/soc/intel/fsp_broadwell_de/smmre... PS1, Line 229: + 1 * MiB;
As the lower address bits are don't care, you decode all addresses in range [0x00000-0xfffff], which […]
Ahh, since in the next line the overall size is computed (tseg_size = tseg_limit - tseg_base;) and tseg_limit is not used anymore, the size computation corrects a possible off-by-one case. That makes sense now.
Patrick Rudolph has submitted this change and it was merged. ( https://review.coreboot.org/c/coreboot/+/30932 )
Change subject: soc/intel/fsp_broadwell_de: Fix TSEG size computation ......................................................................
soc/intel/fsp_broadwell_de: Fix TSEG size computation
The address bits 19:0 of TSEG_LIMIT read as zero, but are ignored on comparison. The result is that the limit is effectively FFFFFh.
Add one MiB to the register value to make TSEG 8MiB instead of 7MiB. Fixes a crash related to SMRR not matching the TSEG region.
Change-Id: I1a625f7bb53a3e90d3cbc0ce16021892861367d8 Signed-off-by: Patrick Rudolph patrick.rudolph@9elements.com Reviewed-on: https://review.coreboot.org/c/30932 Tested-by: build bot (Jenkins) no-reply@coreboot.org Reviewed-by: Arthur Heymans arthur@aheymans.xyz Reviewed-by: Werner Zeh werner.zeh@siemens.com --- M src/soc/intel/fsp_broadwell_de/smmrelocate.c 1 file changed, 5 insertions(+), 1 deletion(-)
Approvals: build bot (Jenkins): Verified Werner Zeh: Looks good to me, approved Arthur Heymans: Looks good to me, approved
diff --git a/src/soc/intel/fsp_broadwell_de/smmrelocate.c b/src/soc/intel/fsp_broadwell_de/smmrelocate.c index f8f98c2..a58744e 100644 --- a/src/soc/intel/fsp_broadwell_de/smmrelocate.c +++ b/src/soc/intel/fsp_broadwell_de/smmrelocate.c @@ -220,9 +220,13 @@ * encompasses the SMRAM range as well as the IED range. * However, the SMRAM available to the handler is 4MiB since the IEDRAM * lives TSEG_BASE + 4MiB. + * + * Note that address bits 19:0 are ignored and not compared. + * The result is that BASE[19:0] is effectively 00000h and LIMIT is + * effectively FFFFFh. */ tseg_base = northbridge_get_base_reg(dev, TSEG_BASE); - tseg_limit = northbridge_get_base_reg(dev, TSEG_LIMIT); + tseg_limit = northbridge_get_base_reg(dev, TSEG_LIMIT) + 1 * MiB; tseg_size = tseg_limit - tseg_base;
params->smram_base = tseg_base;
-
Arthur Heymans (Code Review) -
Patrick Rudolph (Code Review)
-
Werner Zeh (Code Review)