[PATCH 7/6] x86/msr: Introduce msr_{set, clear}_bits() helpers

[Andrew Cooper <andrew.cooper3@citrix.com>]

One reoccuring code pattern is to read an MSR, modify one or more bits,
and write the result back.  Introduce helpers for this purpose.

First, introduce rdmsr_split() and wrmsr_split() which are tiny static inline
wrappers which deal with the MSR value in two 32bit halves.

Next, construct msr_{set,clear}_bits() in terms of the {rdmsr,wrmsr}_split().
The mask operations are deliberately performed as 32bit operations, because
all callers pass in a constant to the mask parameter, and in all current
cases, one of the two operations can be elided.

For MSR_IA32_PSR_L3_QOS_CFG, switch PSR_L3_QOS_CDP_ENABLE from being a bit
position variable to being a plain number.

The resulting C is shorter, and doesn't require a temporary variable.  The
generated ASM is also more efficient, because of avoiding the
packing/unpacking operations.  e.g. the delta in the first hunk is from:

  b9 1b 00 00 00          mov    $0x1b,%ecx
  0f 32                   rdmsr
  48 c1 e2 20             shl    $0x20,%rdx
  48 09 d0                or     %rdx,%rax
  80 e4 f3                and    $0xf3,%ah
  48 89 c2                mov    %rax,%rdx
  48 c1 ea 20             shr    $0x20,%rdx
  0f 30                   wrmsr

to:

  b9 1b 00 00 00          mov    $0x1b,%ecx
  0f 32                   rdmsr
  80 e4 f3                and    $0xf3,%ah
  0f 30                   wrmsr

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Jan Beulich <JBeulich@suse.com>
CC: Wei Liu <wei.liu2@citrix.com>
CC: Roger Pau Monné <roger.pau@citrix.com>
---
 xen/arch/x86/apic.c                 |  8 ++------
 xen/arch/x86/cpu/amd.c              | 19 ++++++-------------
 xen/arch/x86/cpu/centaur.c          |  8 ++------
 xen/arch/x86/cpu/mcheck/mce_intel.c |  7 ++-----
 xen/arch/x86/efi/efi-boot.h         | 11 ++++-------
 xen/arch/x86/hvm/svm/svm.c          |  3 +--
 xen/arch/x86/psr.c                  |  9 +--------
 xen/include/asm-x86/msr.h           | 34 ++++++++++++++++++++++++++++++++++
 xen/include/asm-x86/psr.h           |  2 +-
 9 files changed, 53 insertions(+), 48 deletions(-)

diff --git a/xen/arch/x86/apic.c b/xen/arch/x86/apic.c
index aa677e0..acd8a8a 100644
--- a/xen/arch/x86/apic.c
+++ b/xen/arch/x86/apic.c
@@ -300,12 +300,8 @@ void disable_local_APIC(void)
      */
     apic_write(APIC_SPIV, apic_read(APIC_SPIV) & ~APIC_SPIV_APIC_ENABLED);
 
-    if (enabled_via_apicbase) {
-        uint64_t msr_content;
-        rdmsrl(MSR_APIC_BASE, msr_content);
-        wrmsrl(MSR_APIC_BASE, msr_content &
-               ~(APIC_BASE_ENABLE | APIC_BASE_EXTD));
-    }
+    if ( enabled_via_apicbase )
+        msr_clear_bits(MSR_APIC_BASE, APIC_BASE_ENABLE | APIC_BASE_EXTD);
 
     if ( kexecing && (current_local_apic_mode() != apic_boot_mode) )
     {
diff --git a/xen/arch/x86/cpu/amd.c b/xen/arch/x86/cpu/amd.c
index 458a3fe..e6970ad 100644
--- a/xen/arch/x86/cpu/amd.c
+++ b/xen/arch/x86/cpu/amd.c
@@ -538,11 +538,8 @@ static void init_amd(struct cpuinfo_x86 *c)
 	 * Errata 63 for SH-B3 steppings
 	 * Errata 122 for all steppings (F+ have it disabled by default)
 	 */
-	if (c->x86 == 15) {
-		rdmsrl(MSR_K7_HWCR, value);
-		value |= 1 << 6;
-		wrmsrl(MSR_K7_HWCR, value);
-	}
+	if (c->x86 == 15)
+		msr_set_bits(MSR_K7_HWCR, 1 << 6);
 
 	/*
 	 * Some AMD CPUs duplicate the 3DNow bit in base and extended CPUID
@@ -748,9 +745,7 @@ static void init_amd(struct cpuinfo_x86 *c)
 		 * guests. Prevent this conversion by clearing bit 24 in
 		 * MSR_F10_BU_CFG2.
 		 */
-		rdmsrl(MSR_F10_BU_CFG2, value);
-		value &= ~(1ULL << 24);
-		wrmsrl(MSR_F10_BU_CFG2, value);
+		msr_clear_bits(MSR_F10_BU_CFG2, 1 << 24);
 	}
 
 	/*
@@ -771,11 +766,9 @@ static void init_amd(struct cpuinfo_x86 *c)
 		wrmsrl(MSR_K7_PERFCTR3, 0);
 	}
 
-	if (cpu_has(c, X86_FEATURE_EFRO)) {
-		rdmsr(MSR_K7_HWCR, l, h);
-		l |= (1 << 27); /* Enable read-only APERF/MPERF bit */
-		wrmsr(MSR_K7_HWCR, l, h);
-	}
+	if (cpu_has(c, X86_FEATURE_EFRO))
+		/* Enable read-only APERF/MPERF bit */
+		msr_set_bits(MSR_K7_HWCR, 1 << 27);
 
 	/* Prevent TSC drift in non single-processor, single-core platforms. */
 	if ((smp_processor_id() == 1) && !cpu_has(c, X86_FEATURE_ITSC))
diff --git a/xen/arch/x86/cpu/centaur.c b/xen/arch/x86/cpu/centaur.c
index 1c760be..dfdb5f4 100644
--- a/xen/arch/x86/cpu/centaur.c
+++ b/xen/arch/x86/cpu/centaur.c
@@ -16,25 +16,21 @@
 
 static void init_c3(struct cpuinfo_x86 *c)
 {
-	uint64_t msr_content;
-
 	/* Test for Centaur Extended Feature Flags presence */
 	if (cpuid_eax(0xC0000000) >= 0xC0000001) {
 		u32 tmp = cpuid_edx(0xC0000001);
 
 		/* enable ACE unit, if present and disabled */
 		if ((tmp & (ACE_PRESENT | ACE_ENABLED)) == ACE_PRESENT) {
-			rdmsrl(MSR_VIA_FCR, msr_content);
 			/* enable ACE unit */
-			wrmsrl(MSR_VIA_FCR, msr_content | ACE_FCR);
+			msr_set_bits(MSR_VIA_FCR, ACE_FCR);
 			printk(KERN_INFO "CPU: Enabled ACE h/w crypto\n");
 		}
 
 		/* enable RNG unit, if present and disabled */
 		if ((tmp & (RNG_PRESENT | RNG_ENABLED)) == RNG_PRESENT) {
-			rdmsrl(MSR_VIA_RNG, msr_content);
 			/* enable RNG unit */
-			wrmsrl(MSR_VIA_RNG, msr_content | RNG_ENABLE);
+			msr_set_bits(MSR_VIA_RNG, RNG_ENABLE);
 			printk(KERN_INFO "CPU: Enabled h/w RNG\n");
 		}
 	}
diff --git a/xen/arch/x86/cpu/mcheck/mce_intel.c b/xen/arch/x86/cpu/mcheck/mce_intel.c
index 2d285d0..c5f171d 100644
--- a/xen/arch/x86/cpu/mcheck/mce_intel.c
+++ b/xen/arch/x86/cpu/mcheck/mce_intel.c
@@ -164,11 +164,8 @@ static void intel_init_thermal(struct cpuinfo_x86 *c)
     val |= (APIC_DM_FIXED | APIC_LVT_MASKED);  /* we'll mask till we're ready */
     apic_write(APIC_LVTTHMR, val);
 
-    rdmsrl(MSR_IA32_THERM_INTERRUPT, msr_content);
-    wrmsrl(MSR_IA32_THERM_INTERRUPT, msr_content | 0x03);
-
-    rdmsrl(MSR_IA32_MISC_ENABLE, msr_content);
-    wrmsrl(MSR_IA32_MISC_ENABLE, msr_content | (1ULL<<3));
+    msr_set_bits(MSR_IA32_THERM_INTERRUPT, 0x3);
+    msr_set_bits(MSR_IA32_MISC_ENABLE, 1 << 3);
 
     apic_write(APIC_LVTTHMR, val & ~APIC_LVT_MASKED);
     if ( opt_cpu_info )
diff --git a/xen/arch/x86/efi/efi-boot.h b/xen/arch/x86/efi/efi-boot.h
index 09bb3f4..6619af9 100644
--- a/xen/arch/x86/efi/efi-boot.h
+++ b/xen/arch/x86/efi/efi-boot.h
@@ -229,18 +229,15 @@ static void __init efi_arch_pre_exit_boot(void)
 
 static void __init noreturn efi_arch_post_exit_boot(void)
 {
-    u64 cr4 = XEN_MINIMAL_CR4 & ~X86_CR4_PGE, efer;
+    bool nx = cpuid_ext_features & cpufeat_mask(X86_FEATURE_NX);
+    uint64_t cr4 = XEN_MINIMAL_CR4 & ~X86_CR4_PGE, tmp;
 
     efi_arch_relocate_image(__XEN_VIRT_START - xen_phys_start);
     memcpy((void *)trampoline_phys, trampoline_start, cfg.size);
 
     /* Set system registers and transfer control. */
     asm volatile("pushq $0\n\tpopfq");
-    rdmsrl(MSR_EFER, efer);
-    efer |= EFER_SCE;
-    if ( cpuid_ext_features & cpufeat_mask(X86_FEATURE_NX) )
-        efer |= EFER_NXE;
-    wrmsrl(MSR_EFER, efer);
+    msr_set_bits(MSR_EFER, EFER_SCE | (nx ? EFER_NXE : 0));
     write_cr0(X86_CR0_PE | X86_CR0_MP | X86_CR0_ET | X86_CR0_NE | X86_CR0_WP |
               X86_CR0_AM | X86_CR0_PG);
     asm volatile ( "mov    %[cr4], %%cr4\n\t"
@@ -260,7 +257,7 @@ static void __init noreturn efi_arch_post_exit_boot(void)
                    "movl   %[cs], 8(%%rsp)\n\t"
                    "mov    %[rip], (%%rsp)\n\t"
                    "lretq  %[stkoff]-16"
-                   : [rip] "=&r" (efer/* any dead 64-bit variable */),
+                   : [rip] "=&r" (tmp),
                      [cr4] "+&r" (cr4)
                    : [cr3] "r" (idle_pg_table),
                      [cs] "ir" (__HYPERVISOR_CS),
diff --git a/xen/arch/x86/hvm/svm/svm.c b/xen/arch/x86/hvm/svm/svm.c
index 673a38c..00e3a97 100644
--- a/xen/arch/x86/hvm/svm/svm.c
+++ b/xen/arch/x86/hvm/svm/svm.c
@@ -2484,8 +2484,7 @@ static int svm_is_erratum_383(struct cpu_user_regs *regs)
     for (i = 0; i < nr_mce_banks; i++)
         wrmsrl(MSR_IA32_MCx_STATUS(i), 0ULL);
     
-    rdmsrl(MSR_IA32_MCG_STATUS, msr_content);
-    wrmsrl(MSR_IA32_MCG_STATUS, msr_content & ~(1ULL << 2));
+    msr_clear_bits(MSR_IA32_MCG_STATUS, 1 << 2);
 
     /* flush TLB */
     flush_tlb_mask(v->domain->dirty_cpumask);
diff --git a/xen/arch/x86/psr.c b/xen/arch/x86/psr.c
index 0ba8ef8..c4faa3d 100644
--- a/xen/arch/x86/psr.c
+++ b/xen/arch/x86/psr.c
@@ -342,9 +342,6 @@ static bool cat_init_feature(const struct cpuid_leaf *regs,
         break;
 
     case FEAT_TYPE_L3_CDP:
-    {
-        uint64_t val;
-
         if ( feat->cos_max < 3 )
             return false;
 
@@ -357,12 +354,8 @@ static bool cat_init_feature(const struct cpuid_leaf *regs,
 
         wrmsrl(MSR_IA32_PSR_L3_MASK(0), cat_default_val(feat->cat.cbm_len));
         wrmsrl(MSR_IA32_PSR_L3_MASK(1), cat_default_val(feat->cat.cbm_len));
-        rdmsrl(MSR_IA32_PSR_L3_QOS_CFG, val);
-        wrmsrl(MSR_IA32_PSR_L3_QOS_CFG,
-               val | (1ull << PSR_L3_QOS_CDP_ENABLE_BIT));
-
+        msr_set_bits(MSR_IA32_PSR_L3_QOS_CFG, PSR_L3_QOS_CDP_ENABLE);
         break;
-    }
 
     default:
         return false;
diff --git a/xen/include/asm-x86/msr.h b/xen/include/asm-x86/msr.h
index 8f9f964..cbf92f5 100644
--- a/xen/include/asm-x86/msr.h
+++ b/xen/include/asm-x86/msr.h
@@ -11,6 +11,11 @@
 #include <asm/asm_defns.h>
 #include <asm/cpufeature.h>
 
+static inline void rdmsr_split(unsigned int msr, uint32_t *lo, uint32_t *hi)
+{
+    asm volatile ( "rdmsr" : "=a" (*lo), "=d" (*hi) : "c" (msr) );
+}
+
 #define rdmsr(msr,val1,val2) \
      __asm__ __volatile__("rdmsr" \
 			  : "=a" (val1), "=d" (val2) \
@@ -23,6 +28,11 @@
        val = a__ | ((u64)b__<<32); \
 } while(0)
 
+static inline void wrmsr_split(unsigned int msr, uint32_t lo, uint32_t hi)
+{
+    asm volatile ( "wrmsr" :: "a" (lo), "d" (hi), "c" (msr) );
+}
+
 #define wrmsr(msr,val1,val2) \
      __asm__ __volatile__("wrmsr" \
 			  : /* no outputs */ \
@@ -82,6 +92,30 @@ static inline void msr_split(struct cpu_user_regs *regs, uint64_t val)
     regs->rax = (uint32_t)val;
 }
 
+static inline void msr_set_bits(unsigned int msr, uint64_t mask)
+{
+    uint32_t lo, hi;
+
+    rdmsr_split(msr, &lo, &hi);
+
+    lo |= mask;
+    hi |= (mask >> 32);
+
+    wrmsr_split(msr, lo, hi);
+}
+
+static inline void msr_clear_bits(unsigned int msr, uint64_t mask)
+{
+    uint32_t lo, hi;
+
+    rdmsr_split(msr, &lo, &hi);
+
+    lo &= ~mask;
+    hi &= ~(mask >> 32);
+
+    wrmsr_split(msr, lo, hi);
+}
+
 static inline uint64_t rdtsc(void)
 {
     uint32_t low, high;
diff --git a/xen/include/asm-x86/psr.h b/xen/include/asm-x86/psr.h
index c2257da..6caac67 100644
--- a/xen/include/asm-x86/psr.h
+++ b/xen/include/asm-x86/psr.h
@@ -33,7 +33,7 @@
 #define PSR_CAT_CDP_CAPABILITY          (1u << 2)
 
 /* L3 CDP Enable bit*/
-#define PSR_L3_QOS_CDP_ENABLE_BIT       0x0
+#define PSR_L3_QOS_CDP_ENABLE           (_AC(1, ULL) <<  0)
 
 /* Used by psr_get_info() */
 #define PSR_INFO_IDX_COS_MAX            0
-- 
2.1.4


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel