Change in ...coreboot[master]: cpu/intel/model_1067x: Check for lock bit on IA32_FEATURE_CONTROL

[Arthur Heymans (Code Review)]

Arthur Heymans has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/31255

Change subject: cpu/intel/model_1067x: Check for lock bit on IA32_FEATURE_CONTROL
......................................................................

cpu/intel/model_1067x: Check for lock bit on IA32_FEATURE_CONTROL

df7aecd " cpu/intel: Configure IA32_FEATURE_CONTROL for alternative
SMRR" introduced a regression because it unconditionally writes to
IA32_FEATURE_CONTROL, which if it is already locked results in an
unhandled exception. The lock bit is already set on a system reboot.

Change-Id: I7d2df9e1b9d767809da7a61ccd877c6c40f132eb
Signed-off-by: Arthur Heymans 
---
M src/cpu/intel/model_1067x/mp_init.c
1 file changed, 14 insertions(+), 5 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/55/31255/1

diff --git a/src/cpu/intel/model_1067x/mp_init.c b/src/cpu/intel/model_1067x/mp_init.c
index ab77859..f3890fd 100644
--- a/src/cpu/intel/model_1067x/mp_init.c
+++ b/src/cpu/intel/model_1067x/mp_init.c
@@ -60,11 +60,20 @@
    	/* We don't care if the lock is already setting
    	   as our smm relocation handler is able to handle
    	   setups where SMRR is not enabled here. */
-		if (!IS_ENABLED(CONFIG_SET_IA32_FC_LOCK_BIT))
-			printk(BIOS_INFO,
-			       "Overriding CONFIG_SET_IA32_FC_LOCK_BIT to enable SMRR\n");
-		ia32_ft_ctrl.lo |= (1 << 3) | (1 << 0);
-		wrmsr(IA32_FEATURE_CONTROL, ia32_ft_ctrl);
+		if (ia32_ft_ctrl & (1 << 0)) {
+			/* IA32_FEATURE_CONTROL locked. If we set it again we get an
+			 * illegal instruction
+			 */
+			printk(BIOS_DEBUG, "IA32_FEATURE_CONTROL already locked\n");
+			printk(BIOS_DEBUG, "SMRR status: %senabled\n",
+			       ia32_ft_ctrl & (1 << 3) ? "" : "not ");
+		} else {
+			if (!IS_ENABLED(CONFIG_SET_IA32_FC_LOCK_BIT))
+				printk(BIOS_INFO,
+				       "Overriding CONFIG_SET_IA32_FC_LOCK_BIT to enable SMRR\n");
+			ia32_ft_ctrl.lo |= (1 << 3) | (1 << 0);
+			wrmsr(IA32_FEATURE_CONTROL, ia32_ft_ctrl);
+		}
    } else {
    	set_vmx_and_lock();
    }

[build bot (Jenkins) (Code Review)]

build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31255 )

Change subject: cpu/intel/model_1067x: Check for lock bit on IA32_FEATURE_CONTROL
......................................................................

Patch Set 1:

(1 comment)

https://review.coreboot.org/#/c/31255/1/src/cpu/intel/model_1067x/mp_init.c 
File src/cpu/intel/model_1067x/mp_init.c:

https://review.coreboot.org/#/c/31255/1/src/cpu/intel/model_1067x/mp_init.c@... 
PS1, Line 64: 			/* IA32_FEATURE_CONTROL locked. If we set it again we get an
line over 80 characters

[Arthur Heymans (Code Review)]

Hello Patrick Rudolph, build bot (Jenkins),

I'd like you to reexamine a change. Please visit

https://review.coreboot.org/c/coreboot/+/31255

to look at the new patch set (#2).

Change subject: cpu/intel/model_1067x: Check for lock bit on IA32_FEATURE_CONTROL
......................................................................

cpu/intel/model_1067x: Check for lock bit on IA32_FEATURE_CONTROL

df7aecd " cpu/intel: Configure IA32_FEATURE_CONTROL for alternative
SMRR" introduced a regression because it unconditionally writes to
IA32_FEATURE_CONTROL, which if it is already locked results in an
unhandled exception. The lock bit is already set on a system reboot.

Change-Id: I7d2df9e1b9d767809da7a61ccd877c6c40f132eb
Signed-off-by: Arthur Heymans 
---
M src/cpu/intel/model_1067x/mp_init.c
1 file changed, 14 insertions(+), 5 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/55/31255/2

[build bot (Jenkins) (Code Review)]

build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31255 )

Change subject: cpu/intel/model_1067x: Check for lock bit on IA32_FEATURE_CONTROL
......................................................................

Patch Set 2:

(1 comment)

https://review.coreboot.org/#/c/31255/2/src/cpu/intel/model_1067x/mp_init.c 
File src/cpu/intel/model_1067x/mp_init.c:

https://review.coreboot.org/#/c/31255/2/src/cpu/intel/model_1067x/mp_init.c@... 
PS2, Line 64: 			/* IA32_FEATURE_CONTROL locked. If we set it again we get an
line over 80 characters

[Bill XIE (Code Review)]

Bill XIE has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31255 )

Change subject: cpu/intel/model_1067x: Check for lock bit on IA32_FEATURE_CONTROL
......................................................................

Patch Set 2:

(2 comments)

https://review.coreboot.org/#/c/31255/1/src/cpu/intel/model_1067x/mp_init.c 
File src/cpu/intel/model_1067x/mp_init.c:

https://review.coreboot.org/#/c/31255/1/src/cpu/intel/model_1067x/mp_init.c@... 
PS1, Line 63: ia32_ft_ctrl
It seems supposed to be ia32_ft_ctrl.lo.

https://review.coreboot.org/#/c/31255/1/src/cpu/intel/model_1067x/mp_init.c@... 
PS1, Line 69: ia32_ft_ctrl
It seems supposed to be ia32_ft_ctrl.lo.

[Arthur Heymans (Code Review)]

Hello Patrick Rudolph, build bot (Jenkins),

I'd like you to reexamine a change. Please visit

https://review.coreboot.org/c/coreboot/+/31255

to look at the new patch set (#3).

Change subject: cpu/intel/model_1067x: Check for lock bit on IA32_FEATURE_CONTROL
......................................................................

cpu/intel/model_1067x: Check for lock bit on IA32_FEATURE_CONTROL

df7aecd " cpu/intel: Configure IA32_FEATURE_CONTROL for alternative
SMRR" introduced a regression because it unconditionally writes to
IA32_FEATURE_CONTROL, which if it is already locked results in an
unhandled exception. The lock bit is already set on a system reboot.

Change-Id: I7d2df9e1b9d767809da7a61ccd877c6c40f132eb
Signed-off-by: Arthur Heymans 
---
M src/cpu/intel/model_1067x/mp_init.c
1 file changed, 14 insertions(+), 5 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/55/31255/3

[build bot (Jenkins) (Code Review)]

build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31255 )

Change subject: cpu/intel/model_1067x: Check for lock bit on IA32_FEATURE_CONTROL
......................................................................

Patch Set 3:

(1 comment)

https://review.coreboot.org/#/c/31255/3/src/cpu/intel/model_1067x/mp_init.c 
File src/cpu/intel/model_1067x/mp_init.c:

https://review.coreboot.org/#/c/31255/3/src/cpu/intel/model_1067x/mp_init.c@... 
PS3, Line 64: 			/* IA32_FEATURE_CONTROL locked. If we set it again we get an
line over 80 characters

[Arthur Heymans (Code Review)]

Hello Patrick Rudolph, build bot (Jenkins),

I'd like you to reexamine a change. Please visit

https://review.coreboot.org/c/coreboot/+/31255

to look at the new patch set (#4).

Change subject: cpu/intel/model_1067x: Check for lock bit on IA32_FEATURE_CONTROL
......................................................................

cpu/intel/model_1067x: Check for lock bit on IA32_FEATURE_CONTROL

df7aecd " cpu/intel: Configure IA32_FEATURE_CONTROL for alternative
SMRR" introduced a regression because it unconditionally writes to
IA32_FEATURE_CONTROL, which if it is already locked results in an
unhandled exception. The lock bit is already set on a system reboot.

Change-Id: I7d2df9e1b9d767809da7a61ccd877c6c40f132eb
Signed-off-by: Arthur Heymans 
---
M src/cpu/intel/model_1067x/mp_init.c
1 file changed, 13 insertions(+), 5 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/55/31255/4

[Bill XIE (Code Review)]

Bill XIE has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31255 )

Change subject: cpu/intel/model_1067x: Check for lock bit on IA32_FEATURE_CONTROL
......................................................................

Patch Set 4: Code-Review+1

The regression seems fixed on my x200s, on which the lock bit seems not cleared across reboots.

[Paul Menzel (Code Review)]

Paul Menzel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31255 )

Change subject: cpu/intel/model_1067x: Check for lock bit on IA32_FEATURE_CONTROL
......................................................................

Patch Set 4: Code-Review+1

(2 comments)

https://review.coreboot.org/#/c/31255/4//COMMIT_MSG 
Commit Message:

https://review.coreboot.org/#/c/31255/4//COMMIT_MSG@9 
PS4, Line 9: " cpu/intel
Remove the space after "?

https://review.coreboot.org/#/c/31255/4/src/cpu/intel/model_1067x/mp_init.c 
File src/cpu/intel/model_1067x/mp_init.c:

https://review.coreboot.org/#/c/31255/4/src/cpu/intel/model_1067x/mp_init.c@... 
PS4, Line 66: printk(BIOS_DEBUG, "IA32_FEATURE_CONTROL already locked\n");
Is that expected? If not, a lower log level like NOTICE might be useful.

[Arthur Heymans (Code Review)]

Arthur Heymans has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31255 )

Change subject: cpu/intel/model_1067x: Check for lock bit on IA32_FEATURE_CONTROL
......................................................................

Patch Set 4:

(1 comment)

https://review.coreboot.org/#/c/31255/4/src/cpu/intel/model_1067x/mp_init.c 
File src/cpu/intel/model_1067x/mp_init.c:

https://review.coreboot.org/#/c/31255/4/src/cpu/intel/model_1067x/mp_init.c@... 
PS4, Line 66: printk(BIOS_DEBUG, "IA32_FEATURE_CONTROL already locked\n");
> Is that expected? If not, a lower log level like NOTICE might be useful.

It is expected on reboots at least. It is the same loglevel as cpu/intel/common/common_init.c which does something similar.

[Arthur Heymans (Code Review)]

Hello Patrick Rudolph, Paul Menzel, Bill XIE, build bot (Jenkins),

I'd like you to reexamine a change. Please visit

https://review.coreboot.org/c/coreboot/+/31255

to look at the new patch set (#5).

Change subject: cpu/intel/model_1067x: Check for lock bit on IA32_FEATURE_CONTROL
......................................................................

cpu/intel/model_1067x: Check for lock bit on IA32_FEATURE_CONTROL

df7aecd "cpu/intel: Configure IA32_FEATURE_CONTROL for alternative
SMRR" introduced a regression because it unconditionally writes to
IA32_FEATURE_CONTROL, which if it is already locked results in an
unhandled exception. The lock bit is already set on a system reboot.

Change-Id: I7d2df9e1b9d767809da7a61ccd877c6c40f132eb
Signed-off-by: Arthur Heymans 
---
M src/cpu/intel/model_1067x/mp_init.c
1 file changed, 13 insertions(+), 5 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/55/31255/5

[Patrick Rudolph (Code Review)]

Patrick Rudolph has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31255 )

Change subject: cpu/intel/model_1067x: Check for lock bit on IA32_FEATURE_CONTROL
......................................................................

Patch Set 5: Code-Review+2

[Patrick Georgi (Code Review)]

Patrick Georgi has submitted this change and it was merged. ( https://review.coreboot.org/c/coreboot/+/31255 )

Change subject: cpu/intel/model_1067x: Check for lock bit on IA32_FEATURE_CONTROL
......................................................................

cpu/intel/model_1067x: Check for lock bit on IA32_FEATURE_CONTROL

df7aecd "cpu/intel: Configure IA32_FEATURE_CONTROL for alternative
SMRR" introduced a regression because it unconditionally writes to
IA32_FEATURE_CONTROL, which if it is already locked results in an
unhandled exception. The lock bit is already set on a system reboot.

Change-Id: I7d2df9e1b9d767809da7a61ccd877c6c40f132eb
Signed-off-by: Arthur Heymans 
Reviewed-on: https://review.coreboot.org/c/31255
Reviewed-by: Patrick Rudolph 
Reviewed-by: Bill XIE 
Reviewed-by: Paul Menzel 
Tested-by: build bot (Jenkins) 
---
M src/cpu/intel/model_1067x/mp_init.c
1 file changed, 13 insertions(+), 5 deletions(-)

Approvals:
  build bot (Jenkins): Verified
  Paul Menzel: Looks good to me, but someone else must approve
  Patrick Rudolph: Looks good to me, approved
  Bill XIE: Looks good to me, but someone else must approve

diff --git a/src/cpu/intel/model_1067x/mp_init.c b/src/cpu/intel/model_1067x/mp_init.c
index ab77859..acd56c8 100644
--- a/src/cpu/intel/model_1067x/mp_init.c
+++ b/src/cpu/intel/model_1067x/mp_init.c
@@ -60,11 +60,19 @@
    	/* We don't care if the lock is already setting
    	   as our smm relocation handler is able to handle
    	   setups where SMRR is not enabled here. */
-		if (!IS_ENABLED(CONFIG_SET_IA32_FC_LOCK_BIT))
-			printk(BIOS_INFO,
-			       "Overriding CONFIG_SET_IA32_FC_LOCK_BIT to enable SMRR\n");
-		ia32_ft_ctrl.lo |= (1 << 3) | (1 << 0);
-		wrmsr(IA32_FEATURE_CONTROL, ia32_ft_ctrl);
+		if (ia32_ft_ctrl.lo & (1 << 0)) {
+			/* IA32_FEATURE_CONTROL locked. If we set it again we
+			   get an illegal instruction. */
+			printk(BIOS_DEBUG, "IA32_FEATURE_CONTROL already locked\n");
+			printk(BIOS_DEBUG, "SMRR status: %senabled\n",
+			       ia32_ft_ctrl.lo & (1 << 3) ? "" : "not ");
+		} else {
+			if (!IS_ENABLED(CONFIG_SET_IA32_FC_LOCK_BIT))
+				printk(BIOS_INFO,
+				       "Overriding CONFIG_SET_IA32_FC_LOCK_BIT to enable SMRR\n");
+			ia32_ft_ctrl.lo |= (1 << 3) | (1 << 0);
+			wrmsr(IA32_FEATURE_CONTROL, ia32_ft_ctrl);
+		}
    } else {
    	set_vmx_and_lock();
    }