[PATCH 0/8] x86/msr: Consolidate native/paravirt MSR functions

[PATCH 0/8] x86/msr: Consolidate native/paravirt MSR functions

[Dave Hansen]

This is old cruft, but it appears that having two copies of these
MSR functions is enabling warnings to creep in[1].

I know there's also been some work to pare down the XXL code, but
it's obviously not merged yet and this is a good baby step.

Create helpers that both paravirt and native can use in common code
and remove the paravirt implementations of the helpers. This reduces
the amount of logic that is duplicated in the paravirt code.

The other thing I really like about this is that it puts the
raw=>{native,paravirt} switch in one compact place in the code.

Conceptually:
 -   native: The bare-metal implementation. Might not be usable under
	     paravirt XXL.
 -      raw: The lowest-level function that is always usable. Might
	     be native or paravirt under the hood.
 - paravirt: Always calls the paravirt code, but might end up
	     ultimately calling a native implementation version
	     through paravirt ops.

1. https://lore.kernel.org/all/20260319152210.210854-1-aldocontelk@gmail.com/

 msr.h      |  130 ++++++++++++++++++++++++++++++++-----------------------------
 paravirt.h |   44 --------------------
 2 files changed, 71 insertions(+), 103 deletions(-)

[PATCH 1/8] x86/msr: Use "raw_" names for calls to native_* functions

[Dave Hansen]

From: Dave Hansen <dave.hansen@linux.intel.com>

Currently, the paravirt and native code define a common set of MSR
functions. But, some of the code is duplicated between the two. For
instance, the packing and unpacking of the 64-bit MSR value into two
32-bit values is done in both.

Introduce a new abstraction layer: "raw" calls. For now, define and
use them only in the native case. This lays the foundation of
having the paravirt code also defined "raw" calls.

Signed-off-by: Dave Hansen <dave.hansen@linux.intel.com>
---

 b/arch/x86/include/asm/msr.h |   20 +++++++++++++-------
 1 file changed, 13 insertions(+), 7 deletions(-)

diff -puN arch/x86/include/asm/msr.h~raw_msr_names arch/x86/include/asm/msr.h
--- a/arch/x86/include/asm/msr.h~raw_msr_names	2026-03-20 11:24:18.217756517 -0700
+++ b/arch/x86/include/asm/msr.h	2026-03-20 11:24:18.220756650 -0700
@@ -173,6 +173,12 @@ static inline u64 native_read_pmc(int co
 #include <asm/paravirt.h>
 #else
 #include <linux/errno.h>
+
+#define raw_read_msr		native_read_msr
+#define raw_read_msr_safe	native_read_msr_safe
+#define raw_write_msr		native_write_msr
+#define raw_write_msr_safe	native_write_msr_safe
+
 /*
  * Access to machine-specific registers (available on 586 and better only)
  * Note: the rd* operations modify the parameters directly (without using
@@ -181,35 +187,35 @@ static inline u64 native_read_pmc(int co
 
 #define rdmsr(msr, low, high)					\
 do {								\
-	u64 __val = native_read_msr((msr));			\
+	u64 __val = raw_read_msr((msr));			\
 	(void)((low) = (u32)__val);				\
 	(void)((high) = (u32)(__val >> 32));			\
 } while (0)
 
 static inline void wrmsr(u32 msr, u32 low, u32 high)
 {
-	native_write_msr(msr, (u64)high << 32 | low);
+	raw_write_msr(msr, (u64)high << 32 | low);
 }
 
 #define rdmsrq(msr, val)			\
-	((val) = native_read_msr((msr)))
+	((val) = raw_read_msr((msr)))
 
 static inline void wrmsrq(u32 msr, u64 val)
 {
-	native_write_msr(msr, val);
+	raw_write_msr(msr, val);
 }
 
 /* wrmsr with exception handling */
 static inline int wrmsrq_safe(u32 msr, u64 val)
 {
-	return native_write_msr_safe(msr, val);
+	return raw_write_msr_safe(msr, val);
 }
 
 /* rdmsr with exception handling */
 #define rdmsr_safe(msr, low, high)				\
 ({								\
 	u64 __val;						\
-	int __err = native_read_msr_safe((msr), &__val);	\
+	int __err = raw_read_msr_safe((msr), &__val);		\
 	(*low) = (u32)__val;					\
 	(*high) = (u32)(__val >> 32);				\
 	__err;							\
@@ -217,7 +223,7 @@ static inline int wrmsrq_safe(u32 msr, u
 
 static inline int rdmsrq_safe(u32 msr, u64 *p)
 {
-	return native_read_msr_safe(msr, p);
+	return raw_read_msr_safe(msr, p);
 }
 
 static __always_inline u64 rdpmc(int counter)
_

[PATCH 2/8] x86/msr: Consolidate rdmsr() definitions

[Dave Hansen]

From: Dave Hansen <dave.hansen@linux.intel.com>

The paravirt and native code define very similar rdmsr()
implementations. Move the native one out to common code
and ensure it can find the paravirt implementation by
defining raw_read_msr() there.

Remove the now duplicate paravirt rdmsr().

Signed-off-by: Dave Hansen <dave.hansen@linux.intel.com>
---

 b/arch/x86/include/asm/msr.h      |   20 +++++++++++++-------
 b/arch/x86/include/asm/paravirt.h |    7 -------
 2 files changed, 13 insertions(+), 14 deletions(-)

diff -puN arch/x86/include/asm/msr.h~rdmsr-dups-2 arch/x86/include/asm/msr.h
--- a/arch/x86/include/asm/msr.h~rdmsr-dups-2	2026-03-20 11:24:18.760780632 -0700
+++ b/arch/x86/include/asm/msr.h	2026-03-20 11:24:18.767780943 -0700
@@ -171,6 +171,9 @@ static inline u64 native_read_pmc(int co
 
 #ifdef CONFIG_PARAVIRT_XXL
 #include <asm/paravirt.h>
+
+#define raw_read_msr		paravirt_read_msr
+
 #else
 #include <linux/errno.h>
 
@@ -185,13 +188,6 @@ static inline u64 native_read_pmc(int co
  * pointer indirection), this allows gcc to optimize better
  */
 
-#define rdmsr(msr, low, high)					\
-do {								\
-	u64 __val = raw_read_msr((msr));			\
-	(void)((low) = (u32)__val);				\
-	(void)((high) = (u32)(__val >> 32));			\
-} while (0)
-
 static inline void wrmsr(u32 msr, u32 low, u32 high)
 {
 	raw_write_msr(msr, (u64)high << 32 | low);
@@ -233,6 +229,16 @@ static __always_inline u64 rdpmc(int cou
 
 #endif	/* !CONFIG_PARAVIRT_XXL */
 
+/*
+ * Common paravirt and native helpers:
+ */
+#define rdmsr(msr, low, high)					\
+do {								\
+	u64 __val = raw_read_msr((msr));			\
+	(void)((low) = (u32)__val);				\
+	(void)((high) = (u32)(__val >> 32));			\
+} while (0)
+
 /* Instruction opcode for WRMSRNS supported in binutils >= 2.40 */
 #define ASM_WRMSRNS _ASM_BYTES(0x0f,0x01,0xc6)
 
diff -puN arch/x86/include/asm/paravirt.h~rdmsr-dups-2 arch/x86/include/asm/paravirt.h
--- a/arch/x86/include/asm/paravirt.h~rdmsr-dups-2	2026-03-20 11:24:18.764780810 -0700
+++ b/arch/x86/include/asm/paravirt.h	2026-03-20 11:24:18.767780943 -0700
@@ -161,13 +161,6 @@ static inline int paravirt_write_msr_saf
 	return PVOP_CALL2(int, pv_ops, cpu.write_msr_safe, msr, val);
 }
 
-#define rdmsr(msr, val1, val2)			\
-do {						\
-	u64 _l = paravirt_read_msr(msr);	\
-	val1 = (u32)_l;				\
-	val2 = _l >> 32;			\
-} while (0)
-
 static __always_inline void wrmsr(u32 msr, u32 low, u32 high)
 {
 	paravirt_write_msr(msr, (u64)high << 32 | low);
_

[PATCH 3/8] x86/msr: Consolidate rdmsr_safe() implementations

[Dave Hansen]

From: Dave Hansen <dave.hansen@linux.intel.com>

Use the new "raw_" indirection and consolidate the two rdmsr_safe()
implementations down to one.

Signed-off-by: Dave Hansen <dave.hansen@linux.intel.com>
---

 b/arch/x86/include/asm/msr.h      |   21 +++++++++++----------
 b/arch/x86/include/asm/paravirt.h |   10 ----------
 2 files changed, 11 insertions(+), 20 deletions(-)

diff -puN arch/x86/include/asm/msr.h~rdmsr-dups-4 arch/x86/include/asm/msr.h
--- a/arch/x86/include/asm/msr.h~rdmsr-dups-4	2026-03-20 11:24:19.323805632 -0700
+++ b/arch/x86/include/asm/msr.h	2026-03-20 11:24:19.330805942 -0700
@@ -173,6 +173,7 @@ static inline u64 native_read_pmc(int co
 #include <asm/paravirt.h>
 
 #define raw_read_msr		paravirt_read_msr
+#define raw_read_msr_safe	paravirt_read_msr_safe
 
 #else
 #include <linux/errno.h>
@@ -207,16 +208,6 @@ static inline int wrmsrq_safe(u32 msr, u
 	return raw_write_msr_safe(msr, val);
 }
 
-/* rdmsr with exception handling */
-#define rdmsr_safe(msr, low, high)				\
-({								\
-	u64 __val;						\
-	int __err = raw_read_msr_safe((msr), &__val);		\
-	(*low) = (u32)__val;					\
-	(*high) = (u32)(__val >> 32);				\
-	__err;							\
-})
-
 static inline int rdmsrq_safe(u32 msr, u64 *p)
 {
 	return raw_read_msr_safe(msr, p);
@@ -239,6 +230,16 @@ do {								\
 	(void)((high) = (u32)(__val >> 32));			\
 } while (0)
 
+/* rdmsr with exception handling */
+#define rdmsr_safe(msr, low, high)				\
+({								\
+	u64 __val;						\
+	int __err = raw_read_msr_safe((msr), &__val);		\
+	(*low) = (u32)__val;					\
+	(*high) = (u32)(__val >> 32);				\
+	__err;							\
+})
+
 /* Instruction opcode for WRMSRNS supported in binutils >= 2.40 */
 #define ASM_WRMSRNS _ASM_BYTES(0x0f,0x01,0xc6)
 
diff -puN arch/x86/include/asm/paravirt.h~rdmsr-dups-4 arch/x86/include/asm/paravirt.h
--- a/arch/x86/include/asm/paravirt.h~rdmsr-dups-4	2026-03-20 11:24:19.327805809 -0700
+++ b/arch/x86/include/asm/paravirt.h	2026-03-20 11:24:19.330805942 -0700
@@ -181,16 +181,6 @@ static inline int wrmsrq_safe(u32 msr, u
 	return paravirt_write_msr_safe(msr, val);
 }
 
-/* rdmsr with exception handling */
-#define rdmsr_safe(msr, a, b)				\
-({							\
-	u64 _l;						\
-	int _err = paravirt_read_msr_safe((msr), &_l);	\
-	(*a) = (u32)_l;					\
-	(*b) = (u32)(_l >> 32);				\
-	_err;						\
-})
-
 static __always_inline int rdmsrq_safe(u32 msr, u64 *p)
 {
 	return paravirt_read_msr_safe(msr, p);
_

[PATCH 4/8] x86/msr: Consolidate rdmsrq() implementations

[Dave Hansen]

From: Dave Hansen <dave.hansen@linux.intel.com>

Use the new "raw_" indirection and consolidate the two rdmsrq()
implementations down to one.

The paravirt implementation was probably better, but just stick
with the native one here for consistency.

Signed-off-by: Dave Hansen <dave.hansen@linux.intel.com>
---

 b/arch/x86/include/asm/msr.h      |    6 +++---
 b/arch/x86/include/asm/paravirt.h |    5 -----
 2 files changed, 3 insertions(+), 8 deletions(-)

diff -puN arch/x86/include/asm/msr.h~rdmsr-dups-5 arch/x86/include/asm/msr.h
--- a/arch/x86/include/asm/msr.h~rdmsr-dups-5	2026-03-20 11:24:19.885830584 -0700
+++ b/arch/x86/include/asm/msr.h	2026-03-20 11:24:19.892830895 -0700
@@ -194,9 +194,6 @@ static inline void wrmsr(u32 msr, u32 lo
 	raw_write_msr(msr, (u64)high << 32 | low);
 }
 
-#define rdmsrq(msr, val)			\
-	((val) = raw_read_msr((msr)))
-
 static inline void wrmsrq(u32 msr, u64 val)
 {
 	raw_write_msr(msr, val);
@@ -240,6 +237,9 @@ do {								\
 	__err;							\
 })
 
+#define rdmsrq(msr, val)			\
+	((val) = raw_read_msr((msr)))
+
 /* Instruction opcode for WRMSRNS supported in binutils >= 2.40 */
 #define ASM_WRMSRNS _ASM_BYTES(0x0f,0x01,0xc6)
 
diff -puN arch/x86/include/asm/paravirt.h~rdmsr-dups-5 arch/x86/include/asm/paravirt.h
--- a/arch/x86/include/asm/paravirt.h~rdmsr-dups-5	2026-03-20 11:24:19.888830717 -0700
+++ b/arch/x86/include/asm/paravirt.h	2026-03-20 11:24:19.891830851 -0700
@@ -166,11 +166,6 @@ static __always_inline void wrmsr(u32 ms
 	paravirt_write_msr(msr, (u64)high << 32 | low);
 }
 
-#define rdmsrq(msr, val)			\
-do {						\
-	val = paravirt_read_msr(msr);		\
-} while (0)
-
 static inline void wrmsrq(u32 msr, u64 val)
 {
 	paravirt_write_msr(msr, val);
_

[PATCH 5/8] x86/msr: Consolidate {rd,wr}msr[q]_safe() implementations

[Dave Hansen]

From: Dave Hansen <dave.hansen@linux.intel.com>

These should be very familiar by now. Use the "raw_" indirection
to consolidate the duplicate implementations. Do four at once now
because these are quite straightforward.

Signed-off-by: Dave Hansen <dave.hansen@linux.intel.com>
---

 b/arch/x86/include/asm/msr.h      |   44 +++++++++++++++++++-------------------
 b/arch/x86/include/asm/paravirt.h |   20 -----------------
 2 files changed, 23 insertions(+), 41 deletions(-)

diff -puN arch/x86/include/asm/msr.h~rdmsr-dups-6 arch/x86/include/asm/msr.h
--- a/arch/x86/include/asm/msr.h~rdmsr-dups-6	2026-03-20 11:24:20.448855576 -0700
+++ b/arch/x86/include/asm/msr.h	2026-03-20 11:24:20.455855887 -0700
@@ -174,6 +174,8 @@ static inline u64 native_read_pmc(int co
 
 #define raw_read_msr		paravirt_read_msr
 #define raw_read_msr_safe	paravirt_read_msr_safe
+#define raw_write_msr		paravirt_write_msr
+#define raw_write_msr_safe	paravirt_write_msr_safe
 
 #else
 #include <linux/errno.h>
@@ -189,27 +191,6 @@ static inline u64 native_read_pmc(int co
  * pointer indirection), this allows gcc to optimize better
  */
 
-static inline void wrmsr(u32 msr, u32 low, u32 high)
-{
-	raw_write_msr(msr, (u64)high << 32 | low);
-}
-
-static inline void wrmsrq(u32 msr, u64 val)
-{
-	raw_write_msr(msr, val);
-}
-
-/* wrmsr with exception handling */
-static inline int wrmsrq_safe(u32 msr, u64 val)
-{
-	return raw_write_msr_safe(msr, val);
-}
-
-static inline int rdmsrq_safe(u32 msr, u64 *p)
-{
-	return raw_read_msr_safe(msr, p);
-}
-
 static __always_inline u64 rdpmc(int counter)
 {
 	return native_read_pmc(counter);
@@ -240,6 +221,27 @@ do {								\
 #define rdmsrq(msr, val)			\
 	((val) = raw_read_msr((msr)))
 
+static inline int rdmsrq_safe(u32 msr, u64 *p)
+{
+	return raw_read_msr_safe(msr, p);
+}
+
+/* wrmsr with exception handling */
+static inline int wrmsrq_safe(u32 msr, u64 val)
+{
+	return raw_write_msr_safe(msr, val);
+}
+
+static inline void wrmsr(u32 msr, u32 low, u32 high)
+{
+	raw_write_msr(msr, (u64)high << 32 | low);
+}
+
+static inline void wrmsrq(u32 msr, u64 val)
+{
+	raw_write_msr(msr, val);
+}
+
 /* Instruction opcode for WRMSRNS supported in binutils >= 2.40 */
 #define ASM_WRMSRNS _ASM_BYTES(0x0f,0x01,0xc6)
 
diff -puN arch/x86/include/asm/paravirt.h~rdmsr-dups-6 arch/x86/include/asm/paravirt.h
--- a/arch/x86/include/asm/paravirt.h~rdmsr-dups-6	2026-03-20 11:24:20.452855754 -0700
+++ b/arch/x86/include/asm/paravirt.h	2026-03-20 11:24:20.455855887 -0700
@@ -161,26 +161,6 @@ static inline int paravirt_write_msr_saf
 	return PVOP_CALL2(int, pv_ops, cpu.write_msr_safe, msr, val);
 }
 
-static __always_inline void wrmsr(u32 msr, u32 low, u32 high)
-{
-	paravirt_write_msr(msr, (u64)high << 32 | low);
-}
-
-static inline void wrmsrq(u32 msr, u64 val)
-{
-	paravirt_write_msr(msr, val);
-}
-
-static inline int wrmsrq_safe(u32 msr, u64 val)
-{
-	return paravirt_write_msr_safe(msr, val);
-}
-
-static __always_inline int rdmsrq_safe(u32 msr, u64 *p)
-{
-	return paravirt_read_msr_safe(msr, p);
-}
-
 static __always_inline u64 rdpmc(int counter)
 {
 	return PVOP_CALL1(u64, pv_ops, cpu.read_pmc, counter);
_

[PATCH 6/8] x86/msr: Consolidate rdpmc() implementations

[Dave Hansen]

From: Dave Hansen <dave.hansen@linux.intel.com>

Doing this is debatable. It does not actually remove any code. But,
rdpmc() is the last thing left in the #ifdef CONFIG_PARAVIRT_XXL
block and this seems like nice consistency.

Signed-off-by: Dave Hansen <dave.hansen@linux.intel.com>
---

 b/arch/x86/include/asm/msr.h      |   12 +++++++-----
 b/arch/x86/include/asm/paravirt.h |    2 +-
 2 files changed, 8 insertions(+), 6 deletions(-)

diff -puN arch/x86/include/asm/msr.h~rdmsr-dups-9 arch/x86/include/asm/msr.h
--- a/arch/x86/include/asm/msr.h~rdmsr-dups-9	2026-03-20 11:24:21.013880656 -0700
+++ b/arch/x86/include/asm/msr.h	2026-03-20 11:24:21.020880966 -0700
@@ -176,6 +176,7 @@ static inline u64 native_read_pmc(int co
 #define raw_read_msr_safe	paravirt_read_msr_safe
 #define raw_write_msr		paravirt_write_msr
 #define raw_write_msr_safe	paravirt_write_msr_safe
+#define raw_read_pmc		paravirt_read_pmc
 
 #else
 #include <linux/errno.h>
@@ -184,6 +185,7 @@ static inline u64 native_read_pmc(int co
 #define raw_read_msr_safe	native_read_msr_safe
 #define raw_write_msr		native_write_msr
 #define raw_write_msr_safe	native_write_msr_safe
+#define raw_read_pmc		native_read_pmc
 
 /*
  * Access to machine-specific registers (available on 586 and better only)
@@ -191,11 +193,6 @@ static inline u64 native_read_pmc(int co
  * pointer indirection), this allows gcc to optimize better
  */
 
-static __always_inline u64 rdpmc(int counter)
-{
-	return native_read_pmc(counter);
-}
-
 #endif	/* !CONFIG_PARAVIRT_XXL */
 
 /*
@@ -242,6 +239,11 @@ static inline void wrmsrq(u32 msr, u64 v
 	raw_write_msr(msr, val);
 }
 
+static __always_inline u64 rdpmc(int counter)
+{
+	return raw_read_pmc(counter);
+}
+
 /* Instruction opcode for WRMSRNS supported in binutils >= 2.40 */
 #define ASM_WRMSRNS _ASM_BYTES(0x0f,0x01,0xc6)
 
diff -puN arch/x86/include/asm/paravirt.h~rdmsr-dups-9 arch/x86/include/asm/paravirt.h
--- a/arch/x86/include/asm/paravirt.h~rdmsr-dups-9	2026-03-20 11:24:21.017880833 -0700
+++ b/arch/x86/include/asm/paravirt.h	2026-03-20 11:24:21.020880966 -0700
@@ -161,7 +161,7 @@ static inline int paravirt_write_msr_saf
 	return PVOP_CALL2(int, pv_ops, cpu.write_msr_safe, msr, val);
 }
 
-static __always_inline u64 rdpmc(int counter)
+static __always_inline u64 paravirt_read_pmc(int counter)
 {
 	return PVOP_CALL1(u64, pv_ops, cpu.read_pmc, counter);
 }
_

[PATCH 7/8] x86/msr: Remove old crusty comment

[Dave Hansen]

From: Dave Hansen <dave.hansen@linux.intel.com>

I'm not sure any of this makes sense any more. The kernel only
runs on "586 and better". The comment about gcc optimization is
hopefully decades out of date too.

Really, the only reason to keep the wonky semantics where the
parameters get modified is to avoid all the churn to make them sane.
Not gcc. gcc was probably a bad reason, even back in the day because
MSRs are mostly very slow and have always been very slow. A few
extra bytes of register shuffling was probably never measurable.

Signed-off-by: Dave Hansen <dave.hansen@linux.intel.com>
---

 b/arch/x86/include/asm/msr.h |    6 ------
 1 file changed, 6 deletions(-)

diff -puN arch/x86/include/asm/msr.h~rdmsr-dups-10 arch/x86/include/asm/msr.h
--- a/arch/x86/include/asm/msr.h~rdmsr-dups-10	2026-03-20 11:24:21.571905418 -0700
+++ b/arch/x86/include/asm/msr.h	2026-03-20 11:24:21.574905551 -0700
@@ -187,12 +187,6 @@ static inline u64 native_read_pmc(int co
 #define raw_write_msr_safe	native_write_msr_safe
 #define raw_read_pmc		native_read_pmc
 
-/*
- * Access to machine-specific registers (available on 586 and better only)
- * Note: the rd* operations modify the parameters directly (without using
- * pointer indirection), this allows gcc to optimize better
- */
-
 #endif	/* !CONFIG_PARAVIRT_XXL */
 
 /*
_

[PATCH 8/8] x86/msr: Remove duplicate #include

[Dave Hansen]

From: Dave Hansen <dave.hansen@linux.intel.com>

errno.h is already included for C code at the top of the header.
Zap the duplicate.

Signed-off-by: Dave Hansen <dave.hansen@linux.intel.com>
---

 b/arch/x86/include/asm/msr.h |    1 -
 1 file changed, 1 deletion(-)

diff -puN arch/x86/include/asm/msr.h~rdmsr-dups-11 arch/x86/include/asm/msr.h
--- a/arch/x86/include/asm/msr.h~rdmsr-dups-11	2026-03-20 11:24:22.113929469 -0700
+++ b/arch/x86/include/asm/msr.h	2026-03-20 11:24:22.116929602 -0700
@@ -179,7 +179,6 @@ static inline u64 native_read_pmc(int co
 #define raw_read_pmc		paravirt_read_pmc
 
 #else
-#include <linux/errno.h>
 
 #define raw_read_msr		native_read_msr
 #define raw_read_msr_safe	native_read_msr_safe
_

Re: [PATCH 0/8] x86/msr: Consolidate native/paravirt MSR functions

[Borislav Petkov]

On Fri, Mar 20, 2026 at 12:03:30PM -0700, Dave Hansen wrote:
> This is old cruft, but it appears that having two copies of these
> MSR functions is enabling warnings to creep in[1].
> 
> I know there's also been some work to pare down the XXL code, but
> it's obviously not merged yet and this is a good baby step.
> 
> Create helpers that both paravirt and native can use in common code
> and remove the paravirt implementations of the helpers. This reduces
> the amount of logic that is duplicated in the paravirt code.
> 
> The other thing I really like about this is that it puts the
> raw=>{native,paravirt} switch in one compact place in the code.
> 
> Conceptually:
>  -   native: The bare-metal implementation. Might not be usable under
> 	     paravirt XXL.
>  -      raw: The lowest-level function that is always usable. Might
> 	     be native or paravirt under the hood.

I went through the patchset twice and I kinda get what you're trying to do but
the "raw" thing is confusing as hell.

To me "raw" means, the lowest level of the functionality - something like
__<function_name> with the two underscores. Or three, depending on the
indirection levels.

And those do *only* *raw* instructions - no more indirections.

But then how can "raw" be the lowest level and then still have something else
underneath - native_ and paravirt_?

I *think* this is only a naming issue and with "raw_" you probably wanna say
"native_or_paravirt_" depending on the ifdeffery... but shorter...

If so, I wouldn't call it "raw". I'd say

xx_read_msr()
xx_write_msr()

to denote that the "xx" resolves to either of the two types. But a better
name. I can't think of a good one now but I know that "raw" isn't it...

Hmmm.

-- 
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette

Re: [PATCH 0/8] x86/msr: Consolidate native/paravirt MSR functions

[Dave Hansen]

On 3/20/26 16:33, Borislav Petkov wrote:
...
> xx_read_msr()
> xx_write_msr()
> 
> to denote that the "xx" resolves to either of the two types. But a better
> name. I can't think of a good one now but I know that "raw" isn't it...

Yup, totally agree. I was having a heck of a time finding a good name.

Lemme stew on it for a weekend and see if anything pops out.

Re: [PATCH 0/8] x86/msr: Consolidate native/paravirt MSR functions

[Jürgen Groß]

[-- Attachment #1.1.1: Type: text/plain, Size: 2972 bytes --]

On 21.03.26 00:33, Borislav Petkov wrote:
> On Fri, Mar 20, 2026 at 12:03:30PM -0700, Dave Hansen wrote:
>> This is old cruft, but it appears that having two copies of these
>> MSR functions is enabling warnings to creep in[1].
>>
>> I know there's also been some work to pare down the XXL code, but
>> it's obviously not merged yet and this is a good baby step.
>>
>> Create helpers that both paravirt and native can use in common code
>> and remove the paravirt implementations of the helpers. This reduces
>> the amount of logic that is duplicated in the paravirt code.
>>
>> The other thing I really like about this is that it puts the
>> raw=>{native,paravirt} switch in one compact place in the code.
>>
>> Conceptually:
>>   -   native: The bare-metal implementation. Might not be usable under
>> 	     paravirt XXL.
>>   -      raw: The lowest-level function that is always usable. Might
>> 	     be native or paravirt under the hood.
> 
> I went through the patchset twice and I kinda get what you're trying to do but
> the "raw" thing is confusing as hell.
> 
> To me "raw" means, the lowest level of the functionality - something like
> __<function_name> with the two underscores. Or three, depending on the
> indirection levels.
> 
> And those do *only* *raw* instructions - no more indirections.
> 
> But then how can "raw" be the lowest level and then still have something else
> underneath - native_ and paravirt_?
> 
> I *think* this is only a naming issue and with "raw_" you probably wanna say
> "native_or_paravirt_" depending on the ifdeffery... but shorter...
> 
> If so, I wouldn't call it "raw". I'd say
> 
> xx_read_msr()
> xx_write_msr()
> 
> to denote that the "xx" resolves to either of the two types. But a better
> name. I can't think of a good one now but I know that "raw" isn't it...
> 
> Hmmm.
> 

I'd like to suggest to do a major cleanup of the MSR interfaces. We have too
many of them doing similar things. Some are capable to do tracing, some aren't.
Some are paravirt capable, some aren't. And the names of those functions don't
reflect that at all. We even have multiple functions or macros doing exactly
the same thing, but having different names.

And in future it will be even more complicated due to the write MSR interfaces
needing serializing and non-serializing variants.

My idea would be to have something like:

msr_read()
msr_read_notrace()
msr_write_sync()
msr_write_nosync()
msr_write_sync_notrace()
msr_write_nosync_notrace()

All of those should be paravirt capable and they should be the only "official"
interfaces. They will depend on low-level primitives, but those should be used
only by the official access functions and maybe in some very special places.

I think this should be the first step towards a MSR access consolidation, as
it allows any internal optimizations and changes without further bothering most
of the users.


Juergen

[-- Attachment #1.1.2: OpenPGP public key --]
[-- Type: application/pgp-keys, Size: 3743 bytes --]

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 495 bytes --]

Re: [PATCH 0/8] x86/msr: Consolidate native/paravirt MSR functions

[Juergen Gross]

[-- Attachment #1.1.1: Type: text/plain, Size: 3264 bytes --]

On 21.03.26 07:23, Jürgen Groß wrote:
> On 21.03.26 00:33, Borislav Petkov wrote:
>> On Fri, Mar 20, 2026 at 12:03:30PM -0700, Dave Hansen wrote:
>>> This is old cruft, but it appears that having two copies of these
>>> MSR functions is enabling warnings to creep in[1].
>>>
>>> I know there's also been some work to pare down the XXL code, but
>>> it's obviously not merged yet and this is a good baby step.
>>>
>>> Create helpers that both paravirt and native can use in common code
>>> and remove the paravirt implementations of the helpers. This reduces
>>> the amount of logic that is duplicated in the paravirt code.
>>>
>>> The other thing I really like about this is that it puts the
>>> raw=>{native,paravirt} switch in one compact place in the code.
>>>
>>> Conceptually:
>>>   -   native: The bare-metal implementation. Might not be usable under
>>>          paravirt XXL.
>>>   -      raw: The lowest-level function that is always usable. Might
>>>          be native or paravirt under the hood.
>>
>> I went through the patchset twice and I kinda get what you're trying to do but
>> the "raw" thing is confusing as hell.
>>
>> To me "raw" means, the lowest level of the functionality - something like
>> __<function_name> with the two underscores. Or three, depending on the
>> indirection levels.
>>
>> And those do *only* *raw* instructions - no more indirections.
>>
>> But then how can "raw" be the lowest level and then still have something else
>> underneath - native_ and paravirt_?
>>
>> I *think* this is only a naming issue and with "raw_" you probably wanna say
>> "native_or_paravirt_" depending on the ifdeffery... but shorter...
>>
>> If so, I wouldn't call it "raw". I'd say
>>
>> xx_read_msr()
>> xx_write_msr()
>>
>> to denote that the "xx" resolves to either of the two types. But a better
>> name. I can't think of a good one now but I know that "raw" isn't it...
>>
>> Hmmm.
>>
> 
> I'd like to suggest to do a major cleanup of the MSR interfaces. We have too
> many of them doing similar things. Some are capable to do tracing, some aren't.
> Some are paravirt capable, some aren't. And the names of those functions don't
> reflect that at all. We even have multiple functions or macros doing exactly
> the same thing, but having different names.
> 
> And in future it will be even more complicated due to the write MSR interfaces
> needing serializing and non-serializing variants.
> 
> My idea would be to have something like:
> 
> msr_read()
> msr_read_notrace()
> msr_write_sync()
> msr_write_nosync()
> msr_write_sync_notrace()
> msr_write_nosync_notrace()

Oh, I missed the "safe" variants.

At the same time maybe the "notrace" variants aren't even needed at this
level?

> 
> All of those should be paravirt capable and they should be the only "official"
> interfaces. They will depend on low-level primitives, but those should be used
> only by the official access functions and maybe in some very special places.
> 
> I think this should be the first step towards a MSR access consolidation, as
> it allows any internal optimizations and changes without further bothering most
> of the users.
> 
> 
> Juergen


[-- Attachment #1.1.2: OpenPGP public key --]
[-- Type: application/pgp-keys, Size: 3743 bytes --]

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 495 bytes --]