Change in coreboot[master]: cpu/x86: make set_msr_bit publicly available

[Michael Niewöhner (Code Review)]

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

Change subject: cpu/x86: make set_msr_bit publicly available
......................................................................

cpu/x86: make set_msr_bit publicly available

Haswell and model_2065 implement a static set_msr_bit helper which
should be publicly available instead. Move it to cpu/x86.

Change-Id: I68b314c917f15fc6e5351de1c539d5a3ae646df8
Signed-off-by: Michael Niewöhner 
---
M src/cpu/intel/haswell/finalize.c
M src/cpu/intel/model_2065x/finalize.c
M src/cpu/intel/model_206ax/finalize.c
M src/include/cpu/x86/msr.h
4 files changed, 20 insertions(+), 53 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/38/36338/1

diff --git a/src/cpu/intel/haswell/finalize.c b/src/cpu/intel/haswell/finalize.c
index b75d145..b838f34 100644
--- a/src/cpu/intel/haswell/finalize.c
+++ b/src/cpu/intel/haswell/finalize.c
@@ -22,25 +22,6 @@
  * Document Number 504790
  * Revision 1.6.0, June 2012 */

-#if 0
-static void msr_set_bit(unsigned int reg, unsigned int bit)
-{
-	msr_t msr = rdmsr(reg);
-
-	if (bit < 32) {
-		if (msr.lo & (1 << bit))
-			return;
-		msr.lo |= 1 << bit;
-	} else {
-		if (msr.hi & (1 << (bit - 32)))
-			return;
-		msr.hi |= 1 << (bit - 32);
-	}
-
-	wrmsr(reg, msr);
-}
-#endif
-
 void intel_cpu_haswell_finalize_smm(void)
 {
 #if 0
diff --git a/src/cpu/intel/model_2065x/finalize.c b/src/cpu/intel/model_2065x/finalize.c
index 3c1c2db..a0a3fe2 100644
--- a/src/cpu/intel/model_2065x/finalize.c
+++ b/src/cpu/intel/model_2065x/finalize.c
@@ -23,23 +23,6 @@
  * Document Number 504790
  * Revision 1.6.0, June 2012 */

-static void msr_set_bit(unsigned int reg, unsigned int bit)
-{
-	msr_t msr = rdmsr(reg);
-
-	if (bit < 32) {
-		if (msr.lo & (1 << bit))
-			return;
-		msr.lo |= 1 << bit;
-	} else {
-		if (msr.hi & (1 << (bit - 32)))
-			return;
-		msr.hi |= 1 << (bit - 32);
-	}
-
-	wrmsr(reg, msr);
-}
-
 void intel_model_2065x_finalize_smm(void)
 {
    /* Lock C-State MSR */
diff --git a/src/cpu/intel/model_206ax/finalize.c b/src/cpu/intel/model_206ax/finalize.c
index c9d5376..d51fb21 100644
--- a/src/cpu/intel/model_206ax/finalize.c
+++ b/src/cpu/intel/model_206ax/finalize.c
@@ -23,23 +23,6 @@
  * Document Number 504790
  * Revision 1.6.0, June 2012 */

-static void msr_set_bit(unsigned int reg, unsigned int bit)
-{
-	msr_t msr = rdmsr(reg);
-
-	if (bit < 32) {
-		if (msr.lo & (1 << bit))
-			return;
-		msr.lo |= 1 << bit;
-	} else {
-		if (msr.hi & (1 << (bit - 32)))
-			return;
-		msr.hi |= 1 << (bit - 32);
-	}
-
-	wrmsr(reg, msr);
-}
-
 void intel_model_206ax_finalize_smm(void)
 {
    /* Lock C-State MSR */
diff --git a/src/include/cpu/x86/msr.h b/src/include/cpu/x86/msr.h
index 8c558ce..2710e7f 100644
--- a/src/include/cpu/x86/msr.h
+++ b/src/include/cpu/x86/msr.h
@@ -301,5 +301,25 @@
    return MCA_ERRTYPE_UNKNOWN;
 }

+
+/* Helper for setting single MSR bits */
+static inline void msr_set_bit(unsigned int reg, unsigned int bit)
+{
+	msr_t msr = rdmsr(reg);
+
+	if (bit < 32) {
+		if (msr.lo & (1 << bit))
+			return;
+		msr.lo |= 1 << bit;
+	} else {
+		if (msr.hi & (1 << (bit - 32)))
+			return;
+		msr.hi |= 1 << (bit - 32);
+	}
+
+	wrmsr(reg, msr);
+}
+
+
 #endif /* __ASSEMBLER__ */
 #endif /* CPU_X86_MSR_H */

[Arthur Heymans (Code Review)]

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

Change subject: cpu/x86: make set_msr_bit publicly available
......................................................................

Patch Set 1: Code-Review+1

(1 comment)

https://review.coreboot.org/c/coreboot/+/36338/1/src/include/cpu/x86/msr.h 
File src/include/cpu/x86/msr.h:

https://review.coreboot.org/c/coreboot/+/36338/1/src/include/cpu/x86/msr.h@3... 
PS1, Line 306: static inline
It seems to be referenced quite often. Do you really want static inline here?

[Patrick Georgi (Code Review)]

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

Change subject: cpu/x86: make set_msr_bit publicly available
......................................................................

Patch Set 1:

(1 comment)

https://review.coreboot.org/c/coreboot/+/36338/1/src/include/cpu/x86/msr.h 
File src/include/cpu/x86/msr.h:

https://review.coreboot.org/c/coreboot/+/36338/1/src/include/cpu/x86/msr.h@3... 
PS1, Line 306: static inline
> It seems to be referenced quite often. […]

inline is a hint to the compiler it's free to ignore (if the cost function determines that this is better called than inlined). static is needed since this is a header and we don't want duplicate symbols.

[Michael Niewöhner (Code Review)]

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

Change subject: cpu/x86: make set_msr_bit publicly available
......................................................................

Patch Set 1:

(1 comment)

https://review.coreboot.org/c/coreboot/+/36338/1/src/include/cpu/x86/msr.h 
File src/include/cpu/x86/msr.h:

https://review.coreboot.org/c/coreboot/+/36338/1/src/include/cpu/x86/msr.h@3... 
PS1, Line 306: static inline
> inline is a hint to the compiler it's free to ignore (if the cost function determines that this is b […]

well, static is clear; what shall I do here?

[Michael Niewöhner (Code Review)]

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

Change subject: cpu/x86: make set_msr_bit publicly available
......................................................................

Patch Set 1:

(1 comment)

https://review.coreboot.org/c/coreboot/+/36338/1/src/include/cpu/x86/msr.h 
File src/include/cpu/x86/msr.h:

https://review.coreboot.org/c/coreboot/+/36338/1/src/include/cpu/x86/msr.h@3... 
PS1, Line 306: static inline
> well, static is clear; what shall I do here?

as discussed on IRC, that's ok here

[Nico Huber (Code Review)]

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

Change subject: cpu/x86: make set_msr_bit publicly available
......................................................................

Patch Set 1: Code-Review+2

[Patrick Georgi (Code Review)]

Patrick Georgi has submitted this change. ( https://review.coreboot.org/c/coreboot/+/36338 )

Change subject: cpu/x86: make set_msr_bit publicly available
......................................................................

cpu/x86: make set_msr_bit publicly available

Haswell and model_2065 implement a static set_msr_bit helper which
should be publicly available instead. Move it to cpu/x86.

Change-Id: I68b314c917f15fc6e5351de1c539d5a3ae646df8
Signed-off-by: Michael Niewöhner 
Reviewed-on: https://review.coreboot.org/c/coreboot/+/36338
Reviewed-by: Arthur Heymans 
Reviewed-by: Nico Huber 
Tested-by: build bot (Jenkins) 
---
M src/cpu/intel/haswell/finalize.c
M src/cpu/intel/model_2065x/finalize.c
M src/cpu/intel/model_206ax/finalize.c
M src/include/cpu/x86/msr.h
4 files changed, 20 insertions(+), 53 deletions(-)

Approvals:
  build bot (Jenkins): Verified
  Nico Huber: Looks good to me, approved
  Arthur Heymans: Looks good to me, but someone else must approve

diff --git a/src/cpu/intel/haswell/finalize.c b/src/cpu/intel/haswell/finalize.c
index b75d145..b838f34 100644
--- a/src/cpu/intel/haswell/finalize.c
+++ b/src/cpu/intel/haswell/finalize.c
@@ -22,25 +22,6 @@
  * Document Number 504790
  * Revision 1.6.0, June 2012 */

-#if 0
-static void msr_set_bit(unsigned int reg, unsigned int bit)
-{
-	msr_t msr = rdmsr(reg);
-
-	if (bit < 32) {
-		if (msr.lo & (1 << bit))
-			return;
-		msr.lo |= 1 << bit;
-	} else {
-		if (msr.hi & (1 << (bit - 32)))
-			return;
-		msr.hi |= 1 << (bit - 32);
-	}
-
-	wrmsr(reg, msr);
-}
-#endif
-
 void intel_cpu_haswell_finalize_smm(void)
 {
 #if 0
diff --git a/src/cpu/intel/model_2065x/finalize.c b/src/cpu/intel/model_2065x/finalize.c
index 3c1c2db..a0a3fe2 100644
--- a/src/cpu/intel/model_2065x/finalize.c
+++ b/src/cpu/intel/model_2065x/finalize.c
@@ -23,23 +23,6 @@
  * Document Number 504790
  * Revision 1.6.0, June 2012 */

-static void msr_set_bit(unsigned int reg, unsigned int bit)
-{
-	msr_t msr = rdmsr(reg);
-
-	if (bit < 32) {
-		if (msr.lo & (1 << bit))
-			return;
-		msr.lo |= 1 << bit;
-	} else {
-		if (msr.hi & (1 << (bit - 32)))
-			return;
-		msr.hi |= 1 << (bit - 32);
-	}
-
-	wrmsr(reg, msr);
-}
-
 void intel_model_2065x_finalize_smm(void)
 {
    /* Lock C-State MSR */
diff --git a/src/cpu/intel/model_206ax/finalize.c b/src/cpu/intel/model_206ax/finalize.c
index c9d5376..d51fb21 100644
--- a/src/cpu/intel/model_206ax/finalize.c
+++ b/src/cpu/intel/model_206ax/finalize.c
@@ -23,23 +23,6 @@
  * Document Number 504790
  * Revision 1.6.0, June 2012 */

-static void msr_set_bit(unsigned int reg, unsigned int bit)
-{
-	msr_t msr = rdmsr(reg);
-
-	if (bit < 32) {
-		if (msr.lo & (1 << bit))
-			return;
-		msr.lo |= 1 << bit;
-	} else {
-		if (msr.hi & (1 << (bit - 32)))
-			return;
-		msr.hi |= 1 << (bit - 32);
-	}
-
-	wrmsr(reg, msr);
-}
-
 void intel_model_206ax_finalize_smm(void)
 {
    /* Lock C-State MSR */
diff --git a/src/include/cpu/x86/msr.h b/src/include/cpu/x86/msr.h
index 8c558ce..2710e7f 100644
--- a/src/include/cpu/x86/msr.h
+++ b/src/include/cpu/x86/msr.h
@@ -301,5 +301,25 @@
    return MCA_ERRTYPE_UNKNOWN;
 }

+
+/* Helper for setting single MSR bits */
+static inline void msr_set_bit(unsigned int reg, unsigned int bit)
+{
+	msr_t msr = rdmsr(reg);
+
+	if (bit < 32) {
+		if (msr.lo & (1 << bit))
+			return;
+		msr.lo |= 1 << bit;
+	} else {
+		if (msr.hi & (1 << (bit - 32)))
+			return;
+		msr.hi |= 1 << (bit - 32);
+	}
+
+	wrmsr(reg, msr);
+}
+
+
 #endif /* __ASSEMBLER__ */
 #endif /* CPU_X86_MSR_H */