Angel Pons has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/46491 )
Change subject: sec/intel/txt: Move DPR size to Kconfig ......................................................................
sec/intel/txt: Move DPR size to Kconfig
Instead of hardcoding the size in code, expose it as a Kconfig symbol. This allows platform code to program the size in the MCH DPR register.
Change-Id: I9b9bcfc7ceefea6882f8133a6c3755da2e64a80c Signed-off-by: Angel Pons th3fanbus@gmail.com --- M src/security/intel/txt/Kconfig M src/security/intel/txt/ramstage.c 2 files changed, 10 insertions(+), 1 deletion(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/91/46491/1
diff --git a/src/security/intel/txt/Kconfig b/src/security/intel/txt/Kconfig index 3dd912e..c69a217 100644 --- a/src/security/intel/txt/Kconfig +++ b/src/security/intel/txt/Kconfig @@ -31,6 +31,15 @@ access to Intel resources. Or for some platforms found inside the blob repository.
+config INTEL_TXT_DPR_SIZE + int "DMA Protected Region size in MiB" + range 0 255 + default 3 + help + Specify the size the DPR region needs to have. On at least Haswell, + the MRC does not have an input to specify the size of DPR, so this + field is only used to check if the programmed size is large enough. + config INTEL_TXT_LOGGING bool "Enable verbose logging" help diff --git a/src/security/intel/txt/ramstage.c b/src/security/intel/txt/ramstage.c index f532a2f..8d9f5d9 100644 --- a/src/security/intel/txt/ramstage.c +++ b/src/security/intel/txt/ramstage.c @@ -254,7 +254,7 @@ return; }
- if (dpr.size < 3) { + if (dpr.size < CONFIG_INTEL_TXT_DPR_SIZE) { printk(BIOS_ERR, "TEE-TXT: MCH DPR configured size is too small.\n"); return; }
Arthur Heymans has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/46491 )
Change subject: sec/intel/txt: Move DPR size to Kconfig ......................................................................
Patch Set 1: Code-Review+1
(2 comments)
https://review.coreboot.org/c/coreboot/+/46491/1/src/security/intel/txt/Kcon... File src/security/intel/txt/Kconfig:
https://review.coreboot.org/c/coreboot/+/46491/1/src/security/intel/txt/Kcon... PS1, Line 35: "DMA Protected Region size in MiB" I'm not sure if this should be user configurable? On Haswell you don't have a choice so having this visible can lead to potentially bad checks. On systems where you can configure it, this value will probably be used to configure it. You want this to have some sane value, defined in the SOC Kconfig so also here it does not seem to be a good idea to expose it.
https://review.coreboot.org/c/coreboot/+/46491/1/src/security/intel/txt/Kcon... PS1, Line 36: range 0 255 Heh, I was not aware this existed ^^
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/46491 )
Change subject: sec/intel/txt: Move DPR size to Kconfig ......................................................................
Patch Set 1:
(1 comment)
https://review.coreboot.org/c/coreboot/+/46491/1/src/security/intel/txt/Kcon... File src/security/intel/txt/Kconfig:
https://review.coreboot.org/c/coreboot/+/46491/1/src/security/intel/txt/Kcon... PS1, Line 35: "DMA Protected Region size in MiB"
I'm not sure if this should be user configurable? […]
DPR can be used for things other than TXT, which is why I decided to make this user-configurable. I'm not sure where the original value of 3 MiB comes from. I think SKL would work with just 2 MiB. For Haswell, I hardcoded 4 MiB when I patched the MRC, as this is what reference code uses. 255 MiB is the maximum supported value.
Hello build bot (Jenkins), Arthur Heymans,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/46491
to look at the new patch set (#3).
Change subject: sec/intel/txt: Move DPR size to Kconfig ......................................................................
sec/intel/txt: Move DPR size to Kconfig
Instead of hardcoding the size in code, expose it as a Kconfig symbol. This allows platform code to program the size in the MCH DPR register.
Change-Id: I9b9bcfc7ceefea6882f8133a6c3755da2e64a80c Signed-off-by: Angel Pons th3fanbus@gmail.com --- M src/security/intel/txt/Kconfig M src/security/intel/txt/ramstage.c 2 files changed, 10 insertions(+), 1 deletion(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/91/46491/3
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/46491 )
Change subject: sec/intel/txt: Move DPR size to Kconfig ......................................................................
Patch Set 2:
(1 comment)
https://review.coreboot.org/c/coreboot/+/46491/1/src/security/intel/txt/Kcon... File src/security/intel/txt/Kconfig:
https://review.coreboot.org/c/coreboot/+/46491/1/src/security/intel/txt/Kcon... PS1, Line 35: "DMA Protected Region size in MiB"
DPR can be used for things other than TXT, which is why I decided to make this user-configurable. […]
Dropped the prompt for now.
Arthur Heymans has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/46491 )
Change subject: sec/intel/txt: Move DPR size to Kconfig ......................................................................
Patch Set 3: Code-Review+2
(1 comment)
https://review.coreboot.org/c/coreboot/+/46491/3/src/security/intel/txt/Kcon... File src/security/intel/txt/Kconfig:
https://review.coreboot.org/c/coreboot/+/46491/3/src/security/intel/txt/Kcon... PS3, Line 36: 0 Most documentation I saw say it needs to be at least 3? Maybe that should be reflected here or in the help text?
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/46491 )
Change subject: sec/intel/txt: Move DPR size to Kconfig ......................................................................
Patch Set 4:
(1 comment)
https://review.coreboot.org/c/coreboot/+/46491/3/src/security/intel/txt/Kcon... File src/security/intel/txt/Kconfig:
https://review.coreboot.org/c/coreboot/+/46491/3/src/security/intel/txt/Kcon... PS3, Line 36: 0
Most documentation I saw say it needs to be at least 3? Maybe that should be reflected here or in th […]
For now, this is only used for TXT. However, DPR could be used for anything else, so I chose to specify the range of possible values as a constraint. A DPR size of 0 MiB (that is, DPR disabled) can be programmed in the registers, but TXT won't work.
Angel Pons has submitted this change. ( https://review.coreboot.org/c/coreboot/+/46491 )
Change subject: sec/intel/txt: Move DPR size to Kconfig ......................................................................
sec/intel/txt: Move DPR size to Kconfig
Instead of hardcoding the size in code, expose it as a Kconfig symbol. This allows platform code to program the size in the MCH DPR register.
Change-Id: I9b9bcfc7ceefea6882f8133a6c3755da2e64a80c Signed-off-by: Angel Pons th3fanbus@gmail.com Reviewed-on: https://review.coreboot.org/c/coreboot/+/46491 Tested-by: build bot (Jenkins) no-reply@coreboot.org Reviewed-by: Arthur Heymans arthur@aheymans.xyz --- M src/security/intel/txt/Kconfig M src/security/intel/txt/ramstage.c 2 files changed, 10 insertions(+), 1 deletion(-)
Approvals: build bot (Jenkins): Verified Arthur Heymans: Looks good to me, approved
diff --git a/src/security/intel/txt/Kconfig b/src/security/intel/txt/Kconfig index 3dd912e..b1d0475 100644 --- a/src/security/intel/txt/Kconfig +++ b/src/security/intel/txt/Kconfig @@ -31,6 +31,15 @@ access to Intel resources. Or for some platforms found inside the blob repository.
+config INTEL_TXT_DPR_SIZE + int + range 0 255 + default 3 + help + Specify the size the DPR region needs to have. On at least Haswell, + the MRC does not have an input to specify the size of DPR, so this + field is only used to check if the programmed size is large enough. + config INTEL_TXT_LOGGING bool "Enable verbose logging" help diff --git a/src/security/intel/txt/ramstage.c b/src/security/intel/txt/ramstage.c index f532a2f..8d9f5d9 100644 --- a/src/security/intel/txt/ramstage.c +++ b/src/security/intel/txt/ramstage.c @@ -254,7 +254,7 @@ return; }
- if (dpr.size < 3) { + if (dpr.size < CONFIG_INTEL_TXT_DPR_SIZE) { printk(BIOS_ERR, "TEE-TXT: MCH DPR configured size is too small.\n"); return; }
-
Angel Pons (Code Review) -
Arthur Heymans (Code Review)