Michael Niewöhner has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/35188 )
Change subject: soc/skylake: lock aesni msr ......................................................................
soc/skylake: lock aesni msr
Change-Id: I97a0d3b1b9b0452e929ca07d29c03237b413e521 Signed-off-by: Michael Niewöhner foss@mniewoehner.de --- M src/soc/intel/skylake/cpu.c 1 file changed, 13 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/88/35188/1
diff --git a/src/soc/intel/skylake/cpu.c b/src/soc/intel/skylake/cpu.c index 0d49d28..984c134 100644 --- a/src/soc/intel/skylake/cpu.c +++ b/src/soc/intel/skylake/cpu.c @@ -420,6 +420,16 @@ wrmsr(MSR_EMULATE_PM_TIMER, msr); }
+void cpu_lock_aesni(void) { + msr_t msr; + + msr = rdmsr(MSR_FEATURE_CONFIG); + if ((msr.lo & 1) == 0) { + msr.lo |= 1; /* Lock it */ + wrmsr(MSR_FEATURE_CONFIG, msr); + } +} + /* All CPUs including BSP will run the following function. */ void soc_core_init(struct device *cpu) { @@ -442,6 +452,9 @@ /* Configure Intel Speed Shift */ configure_isst();
+ /* Lock AES-NI MSR */ + cpu_lock_aesni(); + /* Enable ACPI Timer Emulation via MSR 0x121 */ enable_pm_timer_emulation();
build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35188 )
Change subject: soc/skylake: lock aesni msr ......................................................................
Patch Set 1:
(2 comments)
https://review.coreboot.org/c/coreboot/+/35188/1/src/soc/intel/skylake/cpu.c File src/soc/intel/skylake/cpu.c:
https://review.coreboot.org/c/coreboot/+/35188/1/src/soc/intel/skylake/cpu.c... PS1, Line 423: void cpu_lock_aesni(void) { open brace '{' following function definitions go on the next line
https://review.coreboot.org/c/coreboot/+/35188/1/src/soc/intel/skylake/cpu.c... PS1, Line 432: trailing whitespace
Hello Patrick Rudolph, build bot (Jenkins),
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/35188
to look at the new patch set (#2).
Change subject: soc/skylake: lock aesni msr ......................................................................
soc/skylake: lock aesni msr
Change-Id: I97a0d3b1b9b0452e929ca07d29c03237b413e521 Signed-off-by: Michael Niewöhner foss@mniewoehner.de --- M src/soc/intel/skylake/cpu.c 1 file changed, 14 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/88/35188/2
build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35188 )
Change subject: soc/skylake: lock aesni msr ......................................................................
Patch Set 2:
(1 comment)
https://review.coreboot.org/c/coreboot/+/35188/2/src/soc/intel/skylake/cpu.c File src/soc/intel/skylake/cpu.c:
https://review.coreboot.org/c/coreboot/+/35188/2/src/soc/intel/skylake/cpu.c... PS2, Line 433: trailing whitespace
Hello Patrick Rudolph, build bot (Jenkins),
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/35188
to look at the new patch set (#3).
Change subject: soc/skylake: lock aesni msr ......................................................................
soc/skylake: lock aesni msr
Change-Id: I97a0d3b1b9b0452e929ca07d29c03237b413e521 Signed-off-by: Michael Niewöhner foss@mniewoehner.de --- M src/soc/intel/skylake/cpu.c 1 file changed, 14 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/88/35188/3
build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35188 )
Change subject: soc/skylake: lock aesni msr ......................................................................
Patch Set 3:
(1 comment)
https://review.coreboot.org/c/coreboot/+/35188/3/src/soc/intel/skylake/cpu.c File src/soc/intel/skylake/cpu.c:
https://review.coreboot.org/c/coreboot/+/35188/3/src/soc/intel/skylake/cpu.c... PS3, Line 433: trailing whitespace
Hello Patrick Rudolph, build bot (Jenkins),
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/35188
to look at the new patch set (#4).
Change subject: soc/skylake: lock aesni msr ......................................................................
soc/skylake: lock aesni msr
Change-Id: I97a0d3b1b9b0452e929ca07d29c03237b413e521 Signed-off-by: Michael Niewöhner foss@mniewoehner.de --- M src/soc/intel/skylake/cpu.c 1 file changed, 14 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/88/35188/4
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35188 )
Change subject: soc/skylake: lock aesni msr ......................................................................
Patch Set 4:
(2 comments)
https://review.coreboot.org/c/coreboot/+/35188/4//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/35188/4//COMMIT_MSG@7 PS4, Line 7: msr MSR
https://review.coreboot.org/c/coreboot/+/35188/4/src/soc/intel/skylake/cpu.c File src/soc/intel/skylake/cpu.c:
https://review.coreboot.org/c/coreboot/+/35188/4/src/soc/intel/skylake/cpu.c... PS4, Line 429: /* Lock it */ This comment is really obnoxious... maybe explain what locking this is useful for?
Michael Niewöhner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35188 )
Change subject: soc/skylake: lock aesni msr ......................................................................
Patch Set 4:
(1 comment)
https://review.coreboot.org/c/coreboot/+/35188/4/src/soc/intel/skylake/cpu.c File src/soc/intel/skylake/cpu.c:
https://review.coreboot.org/c/coreboot/+/35188/4/src/soc/intel/skylake/cpu.c... PS4, Line 429: /* Lock it */
This comment is really obnoxious... […]
... copied from somewhere below, you can find that multiple times in that file;) Maybe something like "Lock it because the datasheet says so" or "Lock it to prevent unintended disabling" ?
Hello Patrick Rudolph, build bot (Jenkins),
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/35188
to look at the new patch set (#5).
Change subject: soc/skylake: lock AES-NI MSR ......................................................................
soc/skylake: lock AES-NI MSR
Lock AES-NI register to prevent uninteded disabling, as suggested by the MSR datasheet.
Change-Id: I97a0d3b1b9b0452e929ca07d29c03237b413e521 Signed-off-by: Michael Niewöhner foss@mniewoehner.de --- M src/soc/intel/skylake/cpu.c 1 file changed, 15 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/88/35188/5
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35188 )
Change subject: soc/skylake: lock AES-NI MSR ......................................................................
Patch Set 5:
(1 comment)
https://review.coreboot.org/c/coreboot/+/35188/5/src/soc/intel/skylake/cpu.c File src/soc/intel/skylake/cpu.c:
https://review.coreboot.org/c/coreboot/+/35188/5/src/soc/intel/skylake/cpu.c... PS5, Line 428: MSR_FEATURE_CONFIG Before reading the MSR, don't you need to check CPUID to determine if the MSR is really supported?
Michael Niewöhner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35188 )
Change subject: soc/skylake: lock AES-NI MSR ......................................................................
Patch Set 5:
(1 comment)
https://review.coreboot.org/c/coreboot/+/35188/5/src/soc/intel/skylake/cpu.c File src/soc/intel/skylake/cpu.c:
https://review.coreboot.org/c/coreboot/+/35188/5/src/soc/intel/skylake/cpu.c... PS5, Line 428: MSR_FEATURE_CONFIG
Before reading the MSR, don't you need to check CPUID to determine if the MSR is really supported?
No, on skylake and it's successors this is always supported
Michael Niewöhner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35188 )
Change subject: soc/skylake: lock AES-NI MSR ......................................................................
Patch Set 5:
(1 comment)
https://review.coreboot.org/c/coreboot/+/35188/5/src/soc/intel/skylake/cpu.c File src/soc/intel/skylake/cpu.c:
https://review.coreboot.org/c/coreboot/+/35188/5/src/soc/intel/skylake/cpu.c... PS5, Line 428: MSR_FEATURE_CONFIG
No, on skylake and it's successors this is always supported
Done
Michael Niewöhner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35188 )
Change subject: soc/skylake: lock AES-NI MSR ......................................................................
Patch Set 5:
(2 comments)
https://review.coreboot.org/c/coreboot/+/35188/4//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/35188/4//COMMIT_MSG@7 PS4, Line 7: msr
MSR
Done
https://review.coreboot.org/c/coreboot/+/35188/4/src/soc/intel/skylake/cpu.c File src/soc/intel/skylake/cpu.c:
https://review.coreboot.org/c/coreboot/+/35188/4/src/soc/intel/skylake/cpu.c... PS4, Line 429: /* Lock it */
... […]
Done
Hello Patrick Rudolph, Subrata Banik, Angel Pons, build bot (Jenkins), Furquan Shaikh,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/35188
to look at the new patch set (#6).
Change subject: soc/skylake: lock AES-NI MSR ......................................................................
soc/skylake: lock AES-NI MSR
Lock AES-NI register to prevent uninteded disabling, as suggested by the MSR datasheet.
Tested on X11SSM-F
Change-Id: I97a0d3b1b9b0452e929ca07d29c03237b413e521 Signed-off-by: Michael Niewöhner foss@mniewoehner.de --- M src/soc/intel/skylake/cpu.c 1 file changed, 15 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/88/35188/6
Hello Patrick Rudolph, Subrata Banik, Angel Pons, build bot (Jenkins), Furquan Shaikh,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/35188
to look at the new patch set (#7).
Change subject: soc/skylake: lock AES-NI MSR ......................................................................
soc/skylake: lock AES-NI MSR
Lock AES-NI register to prevent uninteded disabling, as suggested by the MSR datasheet.
Change-Id: I97a0d3b1b9b0452e929ca07d29c03237b413e521 Signed-off-by: Michael Niewöhner foss@mniewoehner.de --- M src/soc/intel/skylake/cpu.c 1 file changed, 15 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/88/35188/7
Michael Niewöhner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35188 )
Change subject: soc/skylake: lock AES-NI MSR ......................................................................
Patch Set 7:
*ping* ;-) Any comments on this?
Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35188 )
Change subject: soc/skylake: lock AES-NI MSR ......................................................................
Patch Set 7:
(2 comments)
https://review.coreboot.org/c/coreboot/+/35188/7//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/35188/7//COMMIT_MSG@6 PS7, Line 6: : soc/skylake soc/intel/skylake ?
https://review.coreboot.org/c/coreboot/+/35188/7//COMMIT_MSG@10 PS7, Line 10: MSR datasheet. any idea on whats been the issue if not getting lock ?
Hello Patrick Rudolph, Subrata Banik, Angel Pons, build bot (Jenkins), Nico Huber, Patrick Georgi, Furquan Shaikh,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/35188
to look at the new patch set (#8).
Change subject: soc/intel/skylake: lock AES-NI MSR ......................................................................
soc/intel/skylake: lock AES-NI MSR
Lock AES-NI register to prevent unintended disabling, as suggested by the MSR datasheet.
Change-Id: I97a0d3b1b9b0452e929ca07d29c03237b413e521 Signed-off-by: Michael Niewöhner foss@mniewoehner.de --- M src/soc/intel/skylake/cpu.c 1 file changed, 15 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/88/35188/8
Michael Niewöhner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35188 )
Change subject: soc/intel/skylake: lock AES-NI MSR ......................................................................
Patch Set 8:
(2 comments)
https://review.coreboot.org/c/coreboot/+/35188/7//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/35188/7//COMMIT_MSG@6 PS7, Line 6: : soc/skylake
soc/intel/skylake ?
Done
https://review.coreboot.org/c/coreboot/+/35188/7//COMMIT_MSG@10 PS7, Line 10: MSR datasheet.
any idea on whats been the issue if not getting lock ?
uhm, it could be disabled unintended...
Michael Niewöhner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35188 )
Change subject: soc/intel/skylake: lock AES-NI MSR ......................................................................
Patch Set 8:
(1 comment)
https://review.coreboot.org/c/coreboot/+/35188/7//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/35188/7//COMMIT_MSG@10 PS7, Line 10: MSR datasheet.
uhm, it could be disabled unintended...
the same is done in src/soc/intel/apollolake/cpu.c
Michael Niewöhner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35188 )
Change subject: soc/intel/skylake: lock AES-NI MSR ......................................................................
Patch Set 8:
(1 comment)
https://review.coreboot.org/c/coreboot/+/35188/7//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/35188/7//COMMIT_MSG@10 PS7, Line 10: MSR datasheet.
the same is done in src/soc/intel/apollolake/cpu. […]
Done
Paul Menzel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35188 )
Change subject: soc/intel/skylake: lock AES-NI MSR ......................................................................
Patch Set 8: Code-Review+1
Michael Niewöhner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35188 )
Change subject: soc/intel/skylake: lock AES-NI MSR ......................................................................
Patch Set 8: Code-Review+1
Michael Niewöhner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35188 )
Change subject: soc/intel/skylake: lock AES-NI MSR ......................................................................
Patch Set 8: -Code-Review
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35188 )
Change subject: soc/intel/skylake: lock AES-NI MSR ......................................................................
Patch Set 8:
(1 comment)
https://review.coreboot.org/c/coreboot/+/35188/7//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/35188/7//COMMIT_MSG@10 PS7, Line 10: MSR datasheet.
Done
Is the disabling happening in coreboot or is it some other component?
Michael Niewöhner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35188 )
Change subject: soc/intel/skylake: lock AES-NI MSR ......................................................................
Patch Set 8:
(1 comment)
https://review.coreboot.org/c/coreboot/+/35188/7//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/35188/7//COMMIT_MSG@10 PS7, Line 10: MSR datasheet.
Is the disabling happening in coreboot or is it some other component?
Sorry, what is the question here? it's an MSR
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35188 )
Change subject: soc/intel/skylake: lock AES-NI MSR ......................................................................
Patch Set 8:
(1 comment)
https://review.coreboot.org/c/coreboot/+/35188/7//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/35188/7//COMMIT_MSG@10 PS7, Line 10: MSR datasheet.
Sorry, what is the question here? it's an MSR
You mentioned that this locking is being done to avoid "unintended disabling". What component is doing the unintended disabling? Is that something you observed in practice? Or is this locking being done just as a precaution?
Michael Niewöhner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35188 )
Change subject: soc/intel/skylake: lock AES-NI MSR ......................................................................
Patch Set 8:
(1 comment)
https://review.coreboot.org/c/coreboot/+/35188/7//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/35188/7//COMMIT_MSG@10 PS7, Line 10: MSR datasheet.
You mentioned that this locking is being done to avoid "unintended disabling". […]
Ah, well, this is done as precaution
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35188 )
Change subject: soc/intel/skylake: lock AES-NI MSR ......................................................................
Patch Set 8: Code-Review+2
Patrick Rudolph has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35188 )
Change subject: soc/intel/skylake: lock AES-NI MSR ......................................................................
Patch Set 8: Code-Review-1
According to https://github.com/tianocore/edk2/blob/master/UefiCpuPkg/Library/CpuCommonFe... this should only be written on thread0 of each core. I've to check additional documentation, but it looks like this could #GP on HT enabled platforms. This check should be done for all SoCs, for all MSR accesses in MP init.
Furquan Shaikh has removed a vote from this change. ( https://review.coreboot.org/c/coreboot/+/35188 )
Change subject: soc/intel/skylake: lock AES-NI MSR ......................................................................
Removed Code-Review+2 by Furquan Shaikh furquan@google.com
Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35188 )
Change subject: soc/intel/skylake: lock AES-NI MSR ......................................................................
Patch Set 8:
Patch Set 8: Code-Review-1
According to https://github.com/tianocore/edk2/blob/master/UefiCpuPkg/Library/CpuCommonFe... this should only be written on thread0 of each core. I've to check additional documentation, but it looks like this could #GP on HT enabled platforms. This check should be done for all SoCs, for all MSR accesses in MP init.
Thanks Patrick for pointing out, thats the reason i have asked what problem we are seeing if we like to set this msr then right place might be cpu.c where we are running feature programming
Patrick Rudolph has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35188 )
Change subject: soc/intel/skylake: lock AES-NI MSR ......................................................................
Patch Set 8:
Intel SDM https://www.intel.com/content/dam/www/public/us/en/documents/manuals/64-ia-3... lists that MSR as "Core", not as "Thread". You need to call intel_ht_sibling() or something similar to run it once on every core (I'm looking for a good code example, but can't find it ...).
That might also be the culprit of crashing SGX init with HT enabled...
Michael Niewöhner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35188 )
Change subject: soc/intel/skylake: lock AES-NI MSR ......................................................................
Patch Set 8:
(1 comment)
Patch Set 8:
Intel SDM https://www.intel.com/content/dam/www/public/us/en/documents/manuals/64-ia-3... lists that MSR as "Core", not as "Thread". You need to call intel_ht_sibling() or something similar to run it once on every core (I'm looking for a good code example, but can't find it ...).
That might also be the culprit of crashing SGX init with HT enabled...
Thank you, I will look into this later this day and test SGX+HT again then
https://review.coreboot.org/c/coreboot/+/35188/7//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/35188/7//COMMIT_MSG@10 PS7, Line 10: MSR datasheet.
Ah, well, this is done as precaution
Done
Michael Niewöhner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35188 )
Change subject: soc/intel/skylake: lock AES-NI MSR ......................................................................
Patch Set 9:
This change is ready for review.
Hello Patrick Rudolph, Subrata Banik, Angel Pons, Paul Menzel, build bot (Jenkins), Nico Huber, Furquan Shaikh, Patrick Georgi,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/35188
to look at the new patch set (#10).
Change subject: soc/intel/skylake: lock AES-NI MSR ......................................................................
soc/intel/skylake: lock AES-NI MSR
Lock AES-NI register to prevent unintended disabling, as suggested by the MSR datasheet.
Change-Id: I97a0d3b1b9b0452e929ca07d29c03237b413e521 Signed-off-by: Michael Niewöhner foss@mniewoehner.de --- M src/soc/intel/skylake/Kconfig M src/soc/intel/skylake/cpu.c 2 files changed, 22 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/88/35188/10
Patrick Rudolph has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35188 )
Change subject: soc/intel/skylake: lock AES-NI MSR ......................................................................
Patch Set 10: Code-Review+1
Needs test.
Michael Niewöhner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35188 )
Change subject: soc/intel/skylake: lock AES-NI MSR ......................................................................
Patch Set 10:
(1 comment)
https://review.coreboot.org/c/coreboot/+/35188/9/src/soc/intel/skylake/cpu.c File src/soc/intel/skylake/cpu.c:
https://review.coreboot.org/c/coreboot/+/35188/9/src/soc/intel/skylake/cpu.c... PS9, Line 462: if (!intel_ht_sibling()) {
braces {} are not necessary for single statement blocks
Done
Michael Niewöhner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35188 )
Change subject: soc/intel/skylake: lock AES-NI MSR ......................................................................
Patch Set 10:
ping.
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35188 )
Change subject: soc/intel/skylake: lock AES-NI MSR ......................................................................
Patch Set 10: Code-Review+1
(1 comment)
https://review.coreboot.org/c/coreboot/+/35188/10/src/soc/intel/skylake/cpu.... File src/soc/intel/skylake/cpu.c:
https://review.coreboot.org/c/coreboot/+/35188/10/src/soc/intel/skylake/cpu.... PS10, Line 423: /* Lock AES-NI (MSR_FEATURE_CONFIG) to prevent unintended disabling as suggested in Intel Please add a line break after the /*
Also, the line could be broken earlier to enhance readability.
Michael Niewöhner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35188 )
Change subject: soc/intel/skylake: lock AES-NI MSR ......................................................................
Uploaded patch set 11: Commit message was updated.
Hello Patrick Rudolph, Subrata Banik, Angel Pons, Paul Menzel, build bot (Jenkins), Nico Huber, Furquan Shaikh, Patrick Georgi,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/35188
to look at the new patch set (#11).
Change subject: soc/intel/skylake: lock AES-NI MSR ......................................................................
soc/intel/skylake: lock AES-NI MSR
Lock AES-NI register to prevent unintended disabling, as suggested by the MSR datasheet.
Successfully tested by reading the MSR on X11SSM-F
Change-Id: I97a0d3b1b9b0452e929ca07d29c03237b413e521 Signed-off-by: Michael Niewöhner foss@mniewoehner.de --- M src/soc/intel/skylake/Kconfig M src/soc/intel/skylake/cpu.c 2 files changed, 22 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/88/35188/11
Hello Patrick Rudolph, Subrata Banik, Angel Pons, Paul Menzel, build bot (Jenkins), Nico Huber, Furquan Shaikh, Patrick Georgi,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/35188
to look at the new patch set (#12).
Change subject: soc/intel/skylake: lock AES-NI MSR ......................................................................
soc/intel/skylake: lock AES-NI MSR
Lock AES-NI register to prevent unintended disabling, as suggested by the MSR datasheet.
Successfully tested by reading the MSR on X11SSM-F
Change-Id: I97a0d3b1b9b0452e929ca07d29c03237b413e521 Signed-off-by: Michael Niewöhner foss@mniewoehner.de --- M src/soc/intel/skylake/Kconfig M src/soc/intel/skylake/cpu.c 2 files changed, 23 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/88/35188/12
Michael Niewöhner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35188 )
Change subject: soc/intel/skylake: lock AES-NI MSR ......................................................................
Uploaded patch set 12.
Michael Niewöhner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35188 )
Change subject: soc/intel/skylake: lock AES-NI MSR ......................................................................
Patch Set 12:
(1 comment)
https://review.coreboot.org/c/coreboot/+/35188/10/src/soc/intel/skylake/cpu.... File src/soc/intel/skylake/cpu.c:
https://review.coreboot.org/c/coreboot/+/35188/10/src/soc/intel/skylake/cpu.... PS10, Line 423: /* Lock AES-NI (MSR_FEATURE_CONFIG) to prevent unintended disabling as suggested in Intel
Please add a line break after the /* […]
Done
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35188 )
Change subject: soc/intel/skylake: lock AES-NI MSR ......................................................................
Patch Set 12: Code-Review+2
Patrick Georgi has submitted this change. ( https://review.coreboot.org/c/coreboot/+/35188 )
Change subject: soc/intel/skylake: lock AES-NI MSR ......................................................................
soc/intel/skylake: lock AES-NI MSR
Lock AES-NI register to prevent unintended disabling, as suggested by the MSR datasheet.
Successfully tested by reading the MSR on X11SSM-F
Change-Id: I97a0d3b1b9b0452e929ca07d29c03237b413e521 Signed-off-by: Michael Niewöhner foss@mniewoehner.de Reviewed-on: https://review.coreboot.org/c/coreboot/+/35188 Reviewed-by: Patrick Georgi pgeorgi@google.com Reviewed-by: Nico Huber nico.h@gmx.de Tested-by: build bot (Jenkins) no-reply@coreboot.org --- M src/soc/intel/skylake/Kconfig M src/soc/intel/skylake/cpu.c 2 files changed, 23 insertions(+), 0 deletions(-)
Approvals: build bot (Jenkins): Verified Patrick Georgi: Looks good to me, approved Nico Huber: Looks good to me, approved
diff --git a/src/soc/intel/skylake/Kconfig b/src/soc/intel/skylake/Kconfig index 901e5f9..8bdd9b5 100644 --- a/src/soc/intel/skylake/Kconfig +++ b/src/soc/intel/skylake/Kconfig @@ -31,6 +31,7 @@ select COMMON_FADT select CPU_INTEL_COMMON select CPU_INTEL_FIRMWARE_INTERFACE_TABLE + select CPU_INTEL_COMMON_HYPERTHREADING select C_ENVIRONMENT_BOOTBLOCK select FSP_M_XIP if MAINBOARD_USES_FSP2_0 select FSP_T_XIP if FSP_CAR diff --git a/src/soc/intel/skylake/cpu.c b/src/soc/intel/skylake/cpu.c index 1f9ecad..63142b9 100644 --- a/src/soc/intel/skylake/cpu.c +++ b/src/soc/intel/skylake/cpu.c @@ -420,6 +420,25 @@ wrmsr(MSR_EMULATE_PM_TIMER, msr); }
+/* + * Lock AES-NI (MSR_FEATURE_CONFIG) to prevent unintended disabling + * as suggested in Intel document 325384-070US. + */ +static void cpu_lock_aesni(void) +{ + msr_t msr; + + /* Only run once per core as specified in the MSR datasheet */ + if (intel_ht_sibling()) + return; + + msr = rdmsr(MSR_FEATURE_CONFIG); + if ((msr.lo & 1) == 0) { + msr.lo |= 1; + wrmsr(MSR_FEATURE_CONFIG, msr); + } +} + /* All CPUs including BSP will run the following function. */ void soc_core_init(struct device *cpu) { @@ -444,6 +463,9 @@ /* Configure Intel Speed Shift */ configure_isst();
+ /* Lock AES-NI MSR */ + cpu_lock_aesni(); + /* Enable ACPI Timer Emulation via MSR 0x121 */ enable_pm_timer_emulation();