Patrick Rudolph has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/35312 )
Change subject: soc/intel/skylake: Fix crash in MP init ......................................................................
soc/intel/skylake: Fix crash in MP init
If CPU supports SGX and Hyper-Threading the later needs to be disabled. This fixes random crashes in MP-Init.
I could not find any relation between those two in any datasheet.
To fully migitate L1TF or E2E attacks, hyperthreading should be disabled anyway if SGX is enabled.
Tested on X11SSH-TF.
Change-Id: I7102da028a449c60ca700b3f9ccda9017aa6d6b5 Signed-off-by: Patrick Rudolph patrick.rudolph@9elements.com --- M src/mainboard/supermicro/x11ssh/variants/tf/devicetree.cb M src/soc/intel/skylake/Kconfig 2 files changed, 2 insertions(+), 1 deletion(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/12/35312/1
diff --git a/src/mainboard/supermicro/x11ssh/variants/tf/devicetree.cb b/src/mainboard/supermicro/x11ssh/variants/tf/devicetree.cb index ce6bfa5..acfac29 100644 --- a/src/mainboard/supermicro/x11ssh/variants/tf/devicetree.cb +++ b/src/mainboard/supermicro/x11ssh/variants/tf/devicetree.cb @@ -31,7 +31,7 @@ register "SaGv" = "SaGv_Disabled"
# Disable SGX - register "sgx_enable" = "0" # SGX is broken in coreboot + register "sgx_enable" = "1" # SGX is broken once Hyper-Threading is enabled register "PrmrrSize" = "128 * MiB"
register "pirqa_routing" = "PCH_IRQ11" diff --git a/src/soc/intel/skylake/Kconfig b/src/soc/intel/skylake/Kconfig index 901e5f9..2059618 100644 --- a/src/soc/intel/skylake/Kconfig +++ b/src/soc/intel/skylake/Kconfig @@ -81,6 +81,7 @@ config FSP_HYPERTHREADING bool "Enable Hyper-Threading" depends on MAINBOARD_USES_FSP2_0 + depends on !SOC_INTEL_COMMON_BLOCK_SGX default y
config CPU_INTEL_NUM_FIT_ENTRIES
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35312 )
Change subject: soc/intel/skylake: Fix crash in MP init ......................................................................
Patch Set 1:
Is SGX working with this?
Do note that SGX enablement flow requires reloading the microcode update at some point, not sure if coreboot code does that. (it could only update the microcode once, or do it twice already)
Michael Niewöhner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35312 )
Change subject: soc/intel/skylake: Fix crash in MP init ......................................................................
Patch Set 1:
(2 comments)
https://review.coreboot.org/c/coreboot/+/35312/1//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/35312/1//COMMIT_MSG@12 PS1, Line 12: I could not find any relation between those two in any datasheet. hmmm, with vendor firmware both can be active at the same time
https://review.coreboot.org/c/coreboot/+/35312/1//COMMIT_MSG@14 PS1, Line 14: To fully migitate L1TF or E2E attacks, hyperthreading should be disabled do we really want to disable HT by default?
Michael Niewöhner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35312 )
Change subject: soc/intel/skylake: Fix crash in MP init ......................................................................
Patch Set 1:
(1 comment)
https://review.coreboot.org/c/coreboot/+/35312/1//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/35312/1//COMMIT_MSG@14 PS1, Line 14: To fully migitate L1TF or E2E attacks, hyperthreading should be disabled
do we really want to disable HT by default?
ah, well, sgx_enable only comes into effect when SOC_INTEL_COMMON_BLOCK_SGX=y, so SGX and HT are controllable by config FSP_HYPERTHREADING, which is on by default, right?
Patrick Rudolph has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35312 )
Change subject: soc/intel/skylake: Fix crash in MP init ......................................................................
Patch Set 1: Code-Review-1
(2 comments)
Add runtime detection of SGX and HT.
https://review.coreboot.org/c/coreboot/+/35312/1//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/35312/1//COMMIT_MSG@12 PS1, Line 12: I could not find any relation between those two in any datasheet.
hmmm, with vendor firmware both can be active at the same time
likly a bug in coreboot or 3rdparty blobs
https://review.coreboot.org/c/coreboot/+/35312/1//COMMIT_MSG@14 PS1, Line 14: To fully migitate L1TF or E2E attacks, hyperthreading should be disabled
ah, well, sgx_enable only comes into effect when SOC_INTEL_COMMON_BLOCK_SGX=y, so SGX and HT are con […]
I guess this can be made runtime detectable, as not all CPUs have HT and SGX enabled.
Hello Patrick Rudolph, build bot (Jenkins),
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/35312
to look at the new patch set (#2).
Change subject: soc/intel/common/block/sgx: Fix crash in MP init ......................................................................
soc/intel/common/block/sgx: Fix crash in MP init
On HyperThreading enabled platforms the MSR_PRMRR_PHYS_MASK was written when already locked by the sibling thread. To prevent such race conditions only call the code for one thread, such that the MSRs are only written once per core.
Also add comments that describe the scope of the MSR that is being written to.
Fixes crash in SGX MP init. Tested on Supermicro X11SSH-TF.
Change-Id: I7102da028a449c60ca700b3f9ccda9017aa6d6b5 Signed-off-by: Patrick Rudolph patrick.rudolph@9elements.com --- M src/mainboard/supermicro/x11ssh/variants/tf/devicetree.cb M src/soc/intel/common/block/sgx/Kconfig M src/soc/intel/common/block/sgx/sgx.c 3 files changed, 13 insertions(+), 5 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/12/35312/2
Paul Menzel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35312 )
Change subject: soc/intel/common/block/sgx: Fix crash in MP init ......................................................................
Patch Set 2:
(4 comments)
Wow, nice find!
Could you please separate the mainboard change into its own commit? (Maybe documentation needs to be updated too?)
https://review.coreboot.org/c/coreboot/+/35312/2//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/35312/2//COMMIT_MSG@9 PS2, Line 9: HyperThreading Hyper-threading
https://review.coreboot.org/c/coreboot/+/35312/2//COMMIT_MSG@11 PS2, Line 11: To prevent such race conditions only call the code for one thread, such Please add a blank line above.
https://review.coreboot.org/c/coreboot/+/35312/2/src/soc/intel/common/block/... File src/soc/intel/common/block/sgx/sgx.c:
https://review.coreboot.org/c/coreboot/+/35312/2/src/soc/intel/common/block/... PS2, Line 83: HyperThreading Hyper-threading
https://review.coreboot.org/c/coreboot/+/35312/2/src/soc/intel/common/block/... PS2, Line 218: an on
Hello Patrick Rudolph, build bot (Jenkins),
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/35312
to look at the new patch set (#3).
Change subject: soc/intel/common/block/sgx: Fix crash in MP init ......................................................................
soc/intel/common/block/sgx: Fix crash in MP init
On Hyper-Threading enabled platforms the MSR_PRMRR_PHYS_MASK was written when already locked by the sibling thread.
To prevent such race conditions only call the code for one thread, such that the MSRs are only written once per core.
Also add comments that describe the scope of the MSR that is being written to.
Fixes crash in SGX MP init. Tested on Supermicro X11SSH-TF.
Change-Id: I7102da028a449c60ca700b3f9ccda9017aa6d6b5 Signed-off-by: Patrick Rudolph patrick.rudolph@9elements.com --- M src/mainboard/supermicro/x11ssh/variants/tf/devicetree.cb M src/soc/intel/common/block/sgx/Kconfig M src/soc/intel/common/block/sgx/sgx.c 3 files changed, 13 insertions(+), 5 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/12/35312/3
Hello Patrick Rudolph, build bot (Jenkins),
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/35312
to look at the new patch set (#5).
Change subject: soc/intel/common/block/sgx: Fix crash in MP init ......................................................................
soc/intel/common/block/sgx: Fix crash in MP init
On Hyper-Threading enabled platforms the MSR_PRMRR_PHYS_MASK was written when already locked by the sibling thread.
To prevent such race conditions only call the code for one thread, such that the MSRs are only written once per core.
Also add comments that describe the scope of the MSR that is being written to.
Fixes crash in SGX MP init. Tested on Supermicro X11SSH-TF.
Change-Id: I7102da028a449c60ca700b3f9ccda9017aa6d6b5 Signed-off-by: Patrick Rudolph patrick.rudolph@9elements.com --- M src/mainboard/supermicro/x11ssh/variants/tf/devicetree.cb M src/soc/intel/common/block/sgx/Kconfig M src/soc/intel/common/block/sgx/sgx.c 3 files changed, 13 insertions(+), 5 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/12/35312/5
Hello Patrick Rudolph, build bot (Jenkins),
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/35312
to look at the new patch set (#7).
Change subject: soc/intel/common/block/sgx: Fix crash in MP init ......................................................................
soc/intel/common/block/sgx: Fix crash in MP init
On Hyper-Threading enabled platforms the MSR_PRMRR_PHYS_MASK was written when already locked by the sibling thread.
To prevent such race conditions only call the code for one thread, such that the MSRs are only written once per core.
Also add comments that describe the scope of the MSR that is being written to.
Fixes crash in SGX MP init. Tested on Supermicro X11SSH-TF.
Change-Id: I7102da028a449c60ca700b3f9ccda9017aa6d6b5 Signed-off-by: Patrick Rudolph patrick.rudolph@9elements.com --- M Documentation/mainboard/supermicro/x11ssh-tf.md M src/mainboard/supermicro/x11ssh/variants/tf/devicetree.cb M src/soc/intel/common/block/sgx/Kconfig M src/soc/intel/common/block/sgx/sgx.c 4 files changed, 13 insertions(+), 6 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/12/35312/7
Patrick Rudolph has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35312 )
Change subject: soc/intel/common/block/sgx: Fix crash in MP init ......................................................................
Patch Set 7:
(4 comments)
Updated documentation as well.
https://review.coreboot.org/c/coreboot/+/35312/2//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/35312/2//COMMIT_MSG@9 PS2, Line 9: HyperThreading
Hyper-threading
Done
https://review.coreboot.org/c/coreboot/+/35312/2//COMMIT_MSG@11 PS2, Line 11: To prevent such race conditions only call the code for one thread, such
Please add a blank line above.
Done
https://review.coreboot.org/c/coreboot/+/35312/2/src/soc/intel/common/block/... File src/soc/intel/common/block/sgx/sgx.c:
https://review.coreboot.org/c/coreboot/+/35312/2/src/soc/intel/common/block/... PS2, Line 83: HyperThreading
Hyper-threading
Done
https://review.coreboot.org/c/coreboot/+/35312/2/src/soc/intel/common/block/... PS2, Line 218: an
on
Done
Patrick Rudolph has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35312 )
Change subject: soc/intel/common/block/sgx: Fix crash in MP init ......................................................................
Patch Set 7:
(2 comments)
https://review.coreboot.org/c/coreboot/+/35312/1//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/35312/1//COMMIT_MSG@12 PS1, Line 12: I could not find any relation between those two in any datasheet.
likly a bug in coreboot or 3rdparty blobs
Done
https://review.coreboot.org/c/coreboot/+/35312/1//COMMIT_MSG@14 PS1, Line 14: To fully migitate L1TF or E2E attacks, hyperthreading should be disabled
I guess this can be made runtime detectable, as not all CPUs have HT and SGX enabled.
Done
Hello Patrick Rudolph, build bot (Jenkins),
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/35312
to look at the new patch set (#8).
Change subject: soc/intel/common/block/sgx: Fix crash in MP init ......................................................................
soc/intel/common/block/sgx: Fix crash in MP init
On Hyper-Threading enabled platforms the MSR_PRMRR_PHYS_MASK was written when already locked by the sibling thread.
To prevent such race conditions only call the code for one thread, such that the MSRs are only written once per core.
Also add comments that describe the scope of the MSR that is being written to.
Fixes crash in SGX MP init. Tested on Supermicro X11SSH-TF.
Change-Id: I7102da028a449c60ca700b3f9ccda9017aa6d6b5 Signed-off-by: Patrick Rudolph patrick.rudolph@9elements.com --- M Documentation/mainboard/supermicro/x11ssh-tf.md M src/mainboard/supermicro/x11ssh/variants/tf/devicetree.cb M src/soc/intel/common/block/sgx/Kconfig M src/soc/intel/common/block/sgx/sgx.c 4 files changed, 14 insertions(+), 7 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/12/35312/8
Christian Walter has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35312 )
Change subject: soc/intel/common/block/sgx: Fix crash in MP init ......................................................................
Patch Set 8: Code-Review+1
Paul Menzel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35312 )
Change subject: soc/intel/common/block/sgx: Fix crash in MP init ......................................................................
Patch Set 8: Code-Review+1
Still one commit, but well.
Michael Niewöhner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35312 )
Change subject: soc/intel/common/block/sgx: Fix crash in MP init ......................................................................
Patch Set 8:
as cb:35426 is merged, this need an update/rebase
Michael Niewöhner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35312 )
Change subject: soc/intel/common/block/sgx: Fix crash in MP init ......................................................................
Patch Set 8:
Good message first: this fixes the SGX crash and SGX get's enabled correctly according to chipsec.
The bad news is, this triggers microcode update failures. 1. I get multiple microcode update errors: microcode: updated to revision 0xb4 date=2019-04-01 VMX status: enabled VMX status: enabled VMX status: enabled VMX status: enabled VMX status: enabled VMX status: enabled SGX: MCHECK approved SGX PRMRR SGX activation was successful. IA32_FEATURE_CONTROL already locked microcode: updated to revision 0xb4 date=2019-04-01 microcode: updated to revision 0xb4 date=2019-04-01 microcode: Update failed SGX: MCHECK approved SGX PRMRR SGX: MCHECK approved SGX PRMRR SGX activation was successful. SGX activation was successful.
Full log here: https://paste.xinu.at/4zT/
2. Chipsec complains that PAVPC and TSEGMB are not locked. However, when sgx is disabled these memory regions get locked. I guess FSP does this. I experienced the same behaviour when trying to set MSR_LT_LOCK_MEMORY without SGX enabled, shortly after the point where SGX normally would get enabled. I solved that problem by setting MSR_LT_LOCK_MEMORY in the pch finalize phase. Disclaimer: I do not know, when exactly MSR_LT_LOCK_MEMORY has to be enabled, as it is not documented............. TBD: find out.
Fun fact: even FSP leaves PAVPC and TSEGMB unlocked when SGX is enabled by FSP. I opened an issue on Github: https://github.com/IntelFsp/FSP/issues/34
chipsec fsp: https://paste.xinu.at/Rvz1mD/ chipsec coreboot: https://paste.xinu.at/irjNm/
Michael Niewöhner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35312 )
Change subject: soc/intel/common/block/sgx: Fix crash in MP init ......................................................................
Patch Set 8: Code-Review-1
needs further investigation
Hello Aaron Durbin, Patrick Rudolph, Subrata Banik, Michael Niewöhner, Robbie Zhang, Paul Menzel, Christian Walter, Pratik Prajapati, build bot (Jenkins), Furquan Shaikh,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/35312
to look at the new patch set (#9).
Change subject: soc/intel/common/block/sgx: Fix crash in MP init ......................................................................
soc/intel/common/block/sgx: Fix crash in MP init
On Hyper-Threading enabled platforms the MSR_PRMRR_PHYS_MASK was written when already locked by the sibling thread. In addition it loads microcode updates on all threads.
To prevent such race conditions only call the code on one thread, such that the MSRs are only written once per core and the microcode is only loaded once for each core.
Also add comments that describe the scope of the MSR that is being written to and mention the Intel documents used for reference.
Fixes crash in SGX MP init. Tested on Supermicro X11SSH-TF.
Change-Id: I7102da028a449c60ca700b3f9ccda9017aa6d6b5 Signed-off-by: Patrick Rudolph patrick.rudolph@9elements.com --- M Documentation/mainboard/supermicro/x11-lga1151-series/x11ssh-tf/x11ssh-tf.md M src/mainboard/supermicro/x11-lga1151-series/devicetree.cb M src/soc/intel/common/block/sgx/Kconfig M src/soc/intel/common/block/sgx/sgx.c 4 files changed, 52 insertions(+), 9 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/12/35312/9
Patrick Rudolph has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35312 )
Change subject: soc/intel/common/block/sgx: Fix crash in MP init ......................................................................
Patch Set 9:
Patch Set 8:
Good message first: this fixes the SGX crash and SGX get's enabled correctly according to chipsec.
The bad news is, this triggers microcode update failures.
- I get multiple microcode update errors:
microcode: updated to revision 0xb4 date=2019-04-01 VMX status: enabled VMX status: enabled VMX status: enabled VMX status: enabled VMX status: enabled VMX status: enabled SGX: MCHECK approved SGX PRMRR SGX activation was successful. IA32_FEATURE_CONTROL already locked microcode: updated to revision 0xb4 date=2019-04-01 microcode: updated to revision 0xb4 date=2019-04-01 microcode: Update failed SGX: MCHECK approved SGX PRMRR SGX: MCHECK approved SGX PRMRR SGX activation was successful. SGX activation was successful.
Full log here: https://paste.xinu.at/4zT/
- Chipsec complains that PAVPC and TSEGMB are not locked.
However, when sgx is disabled these memory regions get locked. I guess FSP does this. I experienced the same behaviour when trying to set MSR_LT_LOCK_MEMORY without SGX enabled, shortly after the point where SGX normally would get enabled. I solved that problem by setting MSR_LT_LOCK_MEMORY in the pch finalize phase. Disclaimer: I do not know, when exactly MSR_LT_LOCK_MEMORY has to be enabled, as it is not documented............. TBD: find out.
Fun fact: even FSP leaves PAVPC and TSEGMB unlocked when SGX is enabled by FSP. I opened an issue on Github: https://github.com/IntelFsp/FSP/issues/34
chipsec fsp: https://paste.xinu.at/Rvz1mD/ chipsec coreboot: https://paste.xinu.at/irjNm/
Updated the patch as microcode updates should only be loaded on one thread as per Intel SDM. Also added more comments.
Michael Niewöhner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35312 )
Change subject: soc/intel/common/block/sgx: Fix crash in MP init ......................................................................
Patch Set 9:
Patch Set 9:
Patch Set 8:
Good message first: this fixes the SGX crash and SGX get's enabled correctly according to chipsec.
The bad news is, this triggers microcode update failures.
- I get multiple microcode update errors:
microcode: updated to revision 0xb4 date=2019-04-01 VMX status: enabled VMX status: enabled VMX status: enabled VMX status: enabled VMX status: enabled VMX status: enabled SGX: MCHECK approved SGX PRMRR SGX activation was successful. IA32_FEATURE_CONTROL already locked microcode: updated to revision 0xb4 date=2019-04-01 microcode: updated to revision 0xb4 date=2019-04-01 microcode: Update failed SGX: MCHECK approved SGX PRMRR SGX: MCHECK approved SGX PRMRR SGX activation was successful. SGX activation was successful.
Full log here: https://paste.xinu.at/4zT/
- Chipsec complains that PAVPC and TSEGMB are not locked.
However, when sgx is disabled these memory regions get locked. I guess FSP does this. I experienced the same behaviour when trying to set MSR_LT_LOCK_MEMORY without SGX enabled, shortly after the point where SGX normally would get enabled. I solved that problem by setting MSR_LT_LOCK_MEMORY in the pch finalize phase. Disclaimer: I do not know, when exactly MSR_LT_LOCK_MEMORY has to be enabled, as it is not documented............. TBD: find out.
Fun fact: even FSP leaves PAVPC and TSEGMB unlocked when SGX is enabled by FSP. I opened an issue on Github: https://github.com/IntelFsp/FSP/issues/34
chipsec fsp: https://paste.xinu.at/Rvz1mD/ chipsec coreboot: https://paste.xinu.at/irjNm/
Updated the patch as microcode updates should only be loaded on one thread as per Intel SDM. Also added more comments.
Great, I will test this in the evening.
IMHO the microcode fix from here should got to CB:35739, too.
Any idea about the locking problems?
Arthur Heymans has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35312 )
Change subject: soc/intel/common/block/sgx: Fix crash in MP init ......................................................................
Patch Set 9: Code-Review+1
(1 comment)
https://review.coreboot.org/c/coreboot/+/35312/9/src/soc/intel/common/block/... File src/soc/intel/common/block/sgx/sgx.c:
https://review.coreboot.org/c/coreboot/+/35312/9/src/soc/intel/common/block/... PS9, Line 73: : union { : uint64_t data64; : struct { : uint32_t lo; : uint32_t hi; : } data32; : } prmrr_base, prmrr_mask; unrelated, but this should just be msr_t...
Michael Niewöhner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35312 )
Change subject: soc/intel/common/block/sgx: Fix crash in MP init ......................................................................
Patch Set 9:
(1 comment)
https://review.coreboot.org/c/coreboot/+/35312/9/src/soc/intel/common/block/... File src/soc/intel/common/block/sgx/sgx.c:
https://review.coreboot.org/c/coreboot/+/35312/9/src/soc/intel/common/block/... PS9, Line 247: : /* : * Update just on the first CPU in the core. Other siblings : * get the update automatically according to Document: 253668-060US : * Intel SDM Chapter 9.11.6.3 : * "Update in a System Supporting Intel Hyper-Threading Technology" : * Intel Hyper-Threading Technology has implications on the loading of the : * microcode update. The update must be loaded for each core in a physical : * processor. Thus, for a processor supporting Intel Hyper-Threading : * Technology, only one logical processor per core is required to load the : * microcode update. Each individual logical processor can independently : * load the update. However, MP initialization must provide some mechanism : * (e.g. a software semaphore) to force serialization of microcode update : * loads and to prevent simultaneous load attempts to the same core. : */ : if (!intel_ht_sibling()) { : const void *microcode_patch = intel_mp_current_microcode(); : intel_microcode_load_unlocked(microcode_patch); : } This should got to CB:35739
Patrick Rudolph has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35312 )
Change subject: soc/intel/common/block/sgx: Fix crash in MP init ......................................................................
Patch Set 9:
(2 comments)
https://review.coreboot.org/c/coreboot/+/35312/9/src/soc/intel/common/block/... File src/soc/intel/common/block/sgx/sgx.c:
https://review.coreboot.org/c/coreboot/+/35312/9/src/soc/intel/common/block/... PS9, Line 73: : union { : uint64_t data64; : struct { : uint32_t lo; : uint32_t hi; : } data32; : } prmrr_base, prmrr_mask;
unrelated, but this should just be msr_t...
Done
https://review.coreboot.org/c/coreboot/+/35312/9/src/soc/intel/common/block/... PS9, Line 247: : /* : * Update just on the first CPU in the core. Other siblings : * get the update automatically according to Document: 253668-060US : * Intel SDM Chapter 9.11.6.3 : * "Update in a System Supporting Intel Hyper-Threading Technology" : * Intel Hyper-Threading Technology has implications on the loading of the : * microcode update. The update must be loaded for each core in a physical : * processor. Thus, for a processor supporting Intel Hyper-Threading : * Technology, only one logical processor per core is required to load the : * microcode update. Each individual logical processor can independently : * load the update. However, MP initialization must provide some mechanism : * (e.g. a software semaphore) to force serialization of microcode update : * loads and to prevent simultaneous load attempts to the same core. : */ : if (!intel_ht_sibling()) { : const void *microcode_patch = intel_mp_current_microcode(); : intel_microcode_load_unlocked(microcode_patch); : }
This should got to CB:35739
I don't see why. This commit fixes HT related issue in the SGX code.
Paul Menzel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35312 )
Change subject: soc/intel/common/block/sgx: Fix crash in MP init ......................................................................
Patch Set 9: Code-Review+1
Michael Niewöhner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35312 )
Change subject: soc/intel/common/block/sgx: Fix crash in MP init ......................................................................
Patch Set 9: Code-Review-1
Microcode errors are gone now; the locking problem remains
Patrick Rudolph has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35312 )
Change subject: soc/intel/common/block/sgx: Fix crash in MP init ......................................................................
Patch Set 9:
Patch Set 9: Code-Review-1
Microcode errors are gone now; the locking problem remains
I don't see how that relates to HyperThreading or fixed MP init crashes? I'd fix that in a follow up commit as it seems to be an independent problem.
Michael Niewöhner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35312 )
Change subject: soc/intel/common/block/sgx: Fix crash in MP init ......................................................................
Patch Set 9: Code-Review+1
Patch Set 9:
Patch Set 9: Code-Review-1
Microcode errors are gone now; the locking problem remains
I don't see how that relates to HyperThreading or fixed MP init crashes? I'd fix that in a follow up commit as it seems to be an independent problem.
The problem is: we don't know... We have to investigate this problem, but I give my +1 as a) FSP shows the same behaviour b) this at least makes SGX working without a crash.
Let's talk on IRC on that locking issue
Hello Aaron Durbin, Patrick Rudolph, Subrata Banik, Arthur Heymans, Michael Niewöhner, Robbie Zhang, Paul Menzel, Christian Walter, build bot (Jenkins), Pratik Prajapati, Furquan Shaikh,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/35312
to look at the new patch set (#10).
Change subject: soc/intel/common/block/sgx: Fix crash in MP init ......................................................................
soc/intel/common/block/sgx: Fix crash in MP init
On Hyper-Threading enabled platforms the MSR_PRMRR_PHYS_MASK was written when already locked by the sibling thread. In addition it loads microcode updates on all threads.
To prevent such race conditions only call the code on one thread, such that the MSRs are only written once per core and the microcode is only loaded once for each core.
Also add comments that describe the scope of the MSR that is being written to and mention the Intel documents used for reference.
Fixes crash in SGX MP init. Tested on Supermicro X11SSH-TF.
Change-Id: I7102da028a449c60ca700b3f9ccda9017aa6d6b5 Signed-off-by: Patrick Rudolph patrick.rudolph@9elements.com --- M Documentation/mainboard/supermicro/x11-lga1151-series/x11-lga1151-series.md M src/mainboard/supermicro/x11-lga1151-series/devicetree.cb M src/soc/intel/common/block/sgx/Kconfig M src/soc/intel/common/block/sgx/sgx.c 4 files changed, 52 insertions(+), 9 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/12/35312/10
Patrick Rudolph has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35312 )
Change subject: soc/intel/common/block/sgx: Fix crash in MP init ......................................................................
Patch Set 9:
resolved merge conflict
Christian Walter has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35312 )
Change subject: soc/intel/common/block/sgx: Fix crash in MP init ......................................................................
Patch Set 10: Code-Review+1
Philipp Deppenwiese has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35312 )
Change subject: soc/intel/common/block/sgx: Fix crash in MP init ......................................................................
Patch Set 10: Code-Review+2
Philipp Deppenwiese has submitted this change. ( https://review.coreboot.org/c/coreboot/+/35312 )
Change subject: soc/intel/common/block/sgx: Fix crash in MP init ......................................................................
soc/intel/common/block/sgx: Fix crash in MP init
On Hyper-Threading enabled platforms the MSR_PRMRR_PHYS_MASK was written when already locked by the sibling thread. In addition it loads microcode updates on all threads.
To prevent such race conditions only call the code on one thread, such that the MSRs are only written once per core and the microcode is only loaded once for each core.
Also add comments that describe the scope of the MSR that is being written to and mention the Intel documents used for reference.
Fixes crash in SGX MP init. Tested on Supermicro X11SSH-TF.
Change-Id: I7102da028a449c60ca700b3f9ccda9017aa6d6b5 Signed-off-by: Patrick Rudolph patrick.rudolph@9elements.com Reviewed-on: https://review.coreboot.org/c/coreboot/+/35312 Reviewed-by: Christian Walter christian.walter@9elements.com Reviewed-by: Philipp Deppenwiese zaolin.daisuki@gmail.com Tested-by: build bot (Jenkins) no-reply@coreboot.org --- M Documentation/mainboard/supermicro/x11-lga1151-series/x11-lga1151-series.md M src/mainboard/supermicro/x11-lga1151-series/devicetree.cb M src/soc/intel/common/block/sgx/Kconfig M src/soc/intel/common/block/sgx/sgx.c 4 files changed, 52 insertions(+), 9 deletions(-)
Approvals: build bot (Jenkins): Verified Philipp Deppenwiese: Looks good to me, approved Christian Walter: Looks good to me, but someone else must approve
diff --git a/Documentation/mainboard/supermicro/x11-lga1151-series/x11-lga1151-series.md b/Documentation/mainboard/supermicro/x11-lga1151-series/x11-lga1151-series.md index 6a613e7..8c99527 100644 --- a/Documentation/mainboard/supermicro/x11-lga1151-series/x11-lga1151-series.md +++ b/Documentation/mainboard/supermicro/x11-lga1151-series/x11-lga1151-series.md @@ -29,7 +29,6 @@
These issues apply to all boards. Have a look at the board-specific issues, too.
-- Intel SGX causes secondary APs to crash (disabled for now) when HT is enabled (Fix is WIP CB:35312) - TianoCore doesn't work with Aspeed NGI, as it's text mode only (Fix is WIP CB:35726)
## ToDo diff --git a/src/mainboard/supermicro/x11-lga1151-series/devicetree.cb b/src/mainboard/supermicro/x11-lga1151-series/devicetree.cb index a5ff0c5..ee6aac7 100644 --- a/src/mainboard/supermicro/x11-lga1151-series/devicetree.cb +++ b/src/mainboard/supermicro/x11-lga1151-series/devicetree.cb @@ -17,8 +17,8 @@ register "Device4Enable" = "1" register "SaGv" = "SaGv_Disabled"
- # Disable SGX - register "sgx_enable" = "0" # SGX is broken in coreboot + # Enable SGX + register "sgx_enable" = "1" register "PrmrrSize" = "128 * MiB"
register "pirqa_routing" = "PCH_IRQ11" diff --git a/src/soc/intel/common/block/sgx/Kconfig b/src/soc/intel/common/block/sgx/Kconfig index 0852bfb..ffd501a 100644 --- a/src/soc/intel/common/block/sgx/Kconfig +++ b/src/soc/intel/common/block/sgx/Kconfig @@ -1,5 +1,7 @@ config SOC_INTEL_COMMON_BLOCK_SGX bool + select CPU_INTEL_COMMON + select CPU_INTEL_COMMON_HYPERTHREADING default n help Software Guard eXtension(SGX) Feature. Intel SGX is a set of new CPU diff --git a/src/soc/intel/common/block/sgx/sgx.c b/src/soc/intel/common/block/sgx/sgx.c index 0edf50f..377a719 100644 --- a/src/soc/intel/common/block/sgx/sgx.c +++ b/src/soc/intel/common/block/sgx/sgx.c @@ -17,6 +17,7 @@ #include <cpu/x86/msr.h> #include <cpu/x86/mtrr.h> #include <cpu/intel/microcode.h> +#include <cpu/intel/common/common.h> #include <intelblocks/mp_init.h> #include <intelblocks/msr.h> #include <intelblocks/sgx.h> @@ -54,9 +55,18 @@ } prmrr_base, prmrr_mask; msr_t msr;
- if (!is_sgx_supported()) + /* + * Software Developer's Manual Volume 4: + * Order Number: 335592-068US + * Chapter 2.16.1 + * MSR_PRMRR_PHYS_MASK is in scope "Core" + * MSR_PRMRR_PHYS_BASE is in scope "Core" + * Return if Hyper-Threading is enabled and not thread 0 + */ + if (!is_sgx_supported() || intel_ht_sibling()) return;
+ /* PRMRR_PHYS_MASK is in scope "Core" */ msr = rdmsr(MSR_PRMRR_PHYS_MASK); /* If it is locked don't attempt to write PRMRR MSRs. */ if (msr.lo & PRMRR_PHYS_MASK_LOCK) @@ -109,6 +119,12 @@ { msr_t msr;
+ /* + * Intel 64 and IA-32 ArchitecturesSoftware Developer's ManualVolume 3C + * Order Number: 326019-060US + * Chapter 35.10.2 "Additional MSRs Supported by Intel" + * IA32_FEATURE_CONTROL is in scope "Thread" + */ msr = rdmsr(IA32_FEATURE_CONTROL); /* Only enable it when it is not locked */ if ((msr.lo & FEATURE_CONTROL_LOCK_BIT) == 0) { @@ -121,6 +137,12 @@ { msr_t msr;
+ /* + * Intel 64 and IA-32 ArchitecturesSoftware Developer's ManualVolume 3C + * Order Number: 326019-060US + * Chapter 35.10.2 "Additional MSRs Supported by Intel" + * IA32_FEATURE_CONTROL is in scope "Thread" + */ msr = rdmsr(IA32_FEATURE_CONTROL); /* If it is locked don't attempt to lock it again. */ if ((msr.lo & 1) == 0) { @@ -135,6 +157,7 @@ * for PoC just write '0's to the MSRs. */ msr_t msr = {0, 0};
+ /* SGX_OWNEREPOCH is in scope "Package" */ wrmsr(MSR_SGX_OWNEREPOCH0, msr); wrmsr(MSR_SGX_OWNEREPOCH1, msr); return 0; @@ -175,16 +198,19 @@ return 0; }
+/* + * Configures SGX according to "Intel Software Guard Extensions Technology" + * Document Number: 565432 + */ void sgx_configure(void *unused) { - const void *microcode_patch = intel_mp_current_microcode();
if (!is_sgx_supported() || !is_prmrr_set()) { printk(BIOS_ERR, "SGX: pre-conditions not met\n"); return; }
- /* Enable the SGX feature */ + /* Enable the SGX feature on all threads. */ enable_sgx();
/* Update the owner epoch value */ @@ -194,10 +220,26 @@ /* Ensure to lock memory before reload microcode patch */ cpu_lock_sgx_memory();
- /* Reload the microcode patch */ - intel_microcode_load_unlocked(microcode_patch); + /* + * Update just on the first CPU in the core. Other siblings + * get the update automatically according to Document: 253668-060US + * Intel SDM Chapter 9.11.6.3 + * "Update in a System Supporting Intel Hyper-Threading Technology" + * Intel Hyper-Threading Technology has implications on the loading of the + * microcode update. The update must be loaded for each core in a physical + * processor. Thus, for a processor supporting Intel Hyper-Threading + * Technology, only one logical processor per core is required to load the + * microcode update. Each individual logical processor can independently + * load the update. However, MP initialization must provide some mechanism + * (e.g. a software semaphore) to force serialization of microcode update + * loads and to prevent simultaneous load attempts to the same core. + */ + if (!intel_ht_sibling()) { + const void *microcode_patch = intel_mp_current_microcode(); + intel_microcode_load_unlocked(microcode_patch); + }
- /* Lock the SGX feature */ + /* Lock the SGX feature on all threads. */ lock_sgx();
/* Activate the SGX feature, if PRMRR config was approved by MCHECK */
-
Angel Pons (Code Review) -
Arthur Heymans (Code Review)
-
Christian Walter (Code Review)
-
Michael Niewöhner (Code Review)
-
Patrick Rudolph (Code Review)
-
Paul Menzel (Code Review)
-
Philipp Deppenwiese (Code Review)