Change in coreboot[master]: soc/intel/skylake: use cpu_lt_lock_memory in cpu_lock_sgx_memory

[Michael Niewöhner (Code Review)]

Michael Niewöhner has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/36354 )

Change subject: soc/intel/skylake: use cpu_lt_lock_memory in cpu_lock_sgx_memory
......................................................................

soc/intel/skylake: use cpu_lt_lock_memory in cpu_lock_sgx_memory

Use the new common function to set LT_LOCK_MEMORY prior to SGX
activation.

Change-Id: Iefec0e61c7482a70af60dabc0bec3bf712d8b48a
Signed-off-by: Michael Niewöhner 
---
M src/soc/intel/skylake/cpu.c
1 file changed, 1 insertion(+), 7 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/54/36354/1

diff --git a/src/soc/intel/skylake/cpu.c b/src/soc/intel/skylake/cpu.c
index 63142b9..6e77938 100644
--- a/src/soc/intel/skylake/cpu.c
+++ b/src/soc/intel/skylake/cpu.c
@@ -578,11 +578,5 @@

void cpu_lock_sgx_memory(void)
 {
-	msr_t msr;
-
-	msr = rdmsr(MSR_LT_LOCK_MEMORY);
-	if ((msr.lo & 1) == 0) {
-		msr.lo |= 1; /* Lock it */
-		wrmsr(MSR_LT_LOCK_MEMORY, msr);
-	}
+	cpu_lt_lock_memory(NULL);
 }

[Nico Huber (Code Review)]

Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36354 )

Change subject: soc/intel/skylake: use cpu_lt_lock_memory in cpu_lock_sgx_memory
......................................................................

Patch Set 1:

(1 comment)

https://review.coreboot.org/c/coreboot/+/36354/1/src/soc/intel/skylake/cpu.c 
File src/soc/intel/skylake/cpu.c:

https://review.coreboot.org/c/coreboot/+/36354/1/src/soc/intel/skylake/cpu.c... 
PS1, Line 579: void cpu_lock_sgx_memory(void)
Is this API still needed after the next patch?

[Michael Niewöhner (Code Review)]

Michael Niewöhner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36354 )

Change subject: soc/intel/skylake: use cpu_lt_lock_memory in cpu_lock_sgx_memory
......................................................................

Patch Set 1:

(1 comment)

https://review.coreboot.org/c/coreboot/+/36354/1/src/soc/intel/skylake/cpu.c 
File src/soc/intel/skylake/cpu.c:

https://review.coreboot.org/c/coreboot/+/36354/1/src/soc/intel/skylake/cpu.c... 
PS1, Line 579: void cpu_lock_sgx_memory(void)
> Is this API still needed after the next patch?

Yep, because when SGX is enabled the lock *MUST* happen before SGX init; every platform after skl/kbl does not need to do the lock themselves, which is handled there MCHECK

[Michael Niewöhner (Code Review)]

Michael Niewöhner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36354 )

Change subject: soc/intel/skylake: use cpu_lt_lock_memory in cpu_lock_sgx_memory
......................................................................

Patch Set 1:

(1 comment)

https://review.coreboot.org/c/coreboot/+/36354/1/src/soc/intel/skylake/cpu.c 
File src/soc/intel/skylake/cpu.c:

https://review.coreboot.org/c/coreboot/+/36354/1/src/soc/intel/skylake/cpu.c... 
PS1, Line 579: void cpu_lock_sgx_memory(void)
> Yep, because when SGX is enabled the lock *MUST* happen before SGX init; every platform after skl/kb […]

... so cpu_lock_sgx_memory() must be empty for those platforms

[Nico Huber (Code Review)]

Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36354 )

Change subject: soc/intel/skylake: use cpu_lt_lock_memory in cpu_lock_sgx_memory
......................................................................

Patch Set 1:

(1 comment)

https://review.coreboot.org/c/coreboot/+/36354/1/src/soc/intel/skylake/cpu.c 
File src/soc/intel/skylake/cpu.c:

https://review.coreboot.org/c/coreboot/+/36354/1/src/soc/intel/skylake/cpu.c... 
PS1, Line 579: void cpu_lock_sgx_memory(void)
> ... […]

Ok, I get it. But if there is no other way of doing it, why have a hook
for it. A simple `config ..._SGX_LOCK_MEMORY` should work too.

[Michael Niewöhner (Code Review)]

Michael Niewöhner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36354 )

Change subject: soc/intel/skylake: use cpu_lt_lock_memory in cpu_lock_sgx_memory
......................................................................

Patch Set 1:

(1 comment)

https://review.coreboot.org/c/coreboot/+/36354/1/src/soc/intel/skylake/cpu.c 
File src/soc/intel/skylake/cpu.c:

https://review.coreboot.org/c/coreboot/+/36354/1/src/soc/intel/skylake/cpu.c... 
PS1, Line 579: void cpu_lock_sgx_memory(void)
> ... […]

Done

[Michael Niewöhner (Code Review)]

Michael Niewöhner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36354 )

Change subject: soc/intel/skylake: use cpu_lt_lock_memory in cpu_lock_sgx_memory
......................................................................

Patch Set 1:

(1 comment)

https://review.coreboot.org/c/coreboot/+/36354/1/src/soc/intel/skylake/cpu.c 
File src/soc/intel/skylake/cpu.c:

https://review.coreboot.org/c/coreboot/+/36354/1/src/soc/intel/skylake/cpu.c... 
PS1, Line 579: void cpu_lock_sgx_memory(void)
> Done

oops. not done. good plan, will do it this way

Change in coreboot[master]: soc/intel/common: sgx: use cpu_lt_lock_memory in sgx setup

[Michael Niewöhner (Code Review)]

Michael Niewöhner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36354 )

Change subject: soc/intel/common: sgx: use cpu_lt_lock_memory in sgx setup
......................................................................

Patch Set 2:

This change is ready for review.

Change in coreboot[master]: soc/intel/common: sgx: use cpu_lt_lock_memory in sgx setup

[Nico Huber (Code Review)]

Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36354 )

Change subject: soc/intel/common: sgx: use cpu_lt_lock_memory in sgx setup
......................................................................

Patch Set 2: Code-Review+2

Change in coreboot[master]: soc/intel/common: sgx: use cpu_lt_lock_memory in sgx setup

[Nico Huber (Code Review)]

Nico Huber has submitted this change. ( https://review.coreboot.org/c/coreboot/+/36354 )

Change subject: soc/intel/common: sgx: use cpu_lt_lock_memory in sgx setup
......................................................................

soc/intel/common: sgx: use cpu_lt_lock_memory in sgx setup

Use the new common function to set LT_LOCK_MEMORY prior to SGX
activation based on Kconfig.

Change-Id: Iefec0e61c7482a70af60dabc0bec3bf712d8b48a
Signed-off-by: Michael Niewöhner 
Reviewed-on: https://review.coreboot.org/c/coreboot/+/36354
Tested-by: build bot (Jenkins) 
Reviewed-by: Nico Huber 
---
M src/soc/intel/common/block/sgx/sgx.c
1 file changed, 4 insertions(+), 2 deletions(-)

Approvals:
  build bot (Jenkins): Verified
  Nico Huber: Looks good to me, approved

diff --git a/src/soc/intel/common/block/sgx/sgx.c b/src/soc/intel/common/block/sgx/sgx.c
index b12e6cf..842eb43 100644
--- a/src/soc/intel/common/block/sgx/sgx.c
+++ b/src/soc/intel/common/block/sgx/sgx.c
@@ -18,6 +18,7 @@
 #include <cpu/x86/mtrr.h>
 #include <cpu/intel/microcode.h>
 #include <cpu/intel/common/common.h>
+#include <intelblocks/cpulib.h>
 #include <intelblocks/mp_init.h>
 #include <intelblocks/msr.h>
 #include <intelblocks/sgx.h>
@@ -216,8 +217,9 @@
    if (owner_epoch_update() < 0)
    	return;

-	/* Ensure to lock memory before reload microcode patch */
-	cpu_lock_sgx_memory();
+	if (CONFIG(SOC_INTEL_COMMON_BLOCK_SGX_LOCK_MEMORY))
+		/* Ensure to lock memory before reload microcode patch */
+		cpu_lt_lock_memory(NULL);

/*
     * Update just on the first CPU in the core. Other siblings