Skip to contentSkip to conversation

xen: msr safe cleanup

PATCH In review Christoph Egger 2 participants 3 messages Jun 11, 2010

lore
Share & tools
lore
#0 lore raw

PATCH 1/1 xen: msr safe cleanup

Hi!

Attached patch cleans up msr-handling with rdmsr_safe/wrmsr_safe. This is part is extracted from the big msr cleanup patch.

Signature · 6 lines
---to satisfy European Law for business letters:
Advanced Micro Devices GmbH
Einsteinring 24, 85609 Dornach b. Muenchen
Geschaeftsfuehrer: Andrew Bowd, Thomas M. McCoy, Giuliano Meroni
Sitz: Dornach, Gemeinde Aschheim, Landkreis Muenchen
Registergericht Muenchen, HRB Nr. 43632
xen/arch/x86/cpu/amd.c +6 -6
diff -r 04e81a65d27e xen/arch/x86/cpu/amd.c
@@ -235,18 +235,18 @@ int force_mwait __cpuinitdata;
 
 static void disable_c1e(void *unused)
 {
-	u32 lo, hi;
+	uint64_t msr_content;
 
 	/*
 	 * Disable C1E mode, as the APIC timer stops in that mode.
 	 * The MSR does not exist in all FamilyF CPUs (only Rev F and above),
 	 * but we safely catch the #GP in that case.
 	 */
-	if ((rdmsr_safe(MSR_K8_ENABLE_C1E, lo, hi) == 0) &&
-	    (lo & (3u << 27)) &&
-	    (wrmsr_safe(MSR_K8_ENABLE_C1E, lo & ~(3u << 27), hi) != 0))
-		printk(KERN_ERR "Failed to disable C1E on CPU#%u (%08x)\n",
-		       smp_processor_id(), lo);
+	if ((rdmsr_safe(MSR_K8_ENABLE_C1E, msr_content) == 0) &&
+	    (msr_content & (3u << 27)) &&
+	    (wrmsr_safe(MSR_K8_ENABLE_C1E, msr_content & ~(3u << 27)) != 0))
+		printk(KERN_ERR "Failed to disable C1E on CPU#%u (%16"PRIx64")\n",
+		       smp_processor_id(), msr_content);
 }
 
 static void check_disable_c1e(unsigned int port, u8 value)
xen/arch/x86/cpu/mtrr/generic.c +4 -1
diff -r 04e81a65d27e xen/arch/x86/cpu/mtrr/generic.c
@@ -107,7 +107,10 @@ void __init mtrr_state_warn(void)
    worth it because the best error handling is to ignore it. */
 void mtrr_wrmsr(unsigned msr, unsigned a, unsigned b)
 {
-	if (wrmsr_safe(msr, a, b) < 0)
+	uint64_t msr_content;
+
+	msr_content = ((uint64_t)b << 32) | a;
+	if (wrmsr_safe(msr, msr_content) < 0)
 		printk(KERN_ERR
 			"MTRR: CPU %u: Writing MSR %x to %x:%x failed\n",
 			smp_processor_id(), msr, a, b);
xen/arch/x86/hvm/svm/svm.c +10 -15
diff -r 04e81a65d27e xen/arch/x86/hvm/svm/svm.c
@@ -857,14 +857,14 @@ static void svm_init_erratum_383(struct 
 
 static int svm_cpu_up(void)
 {
-    u32 eax, edx, phys_hsa_lo, phys_hsa_hi;   
-    u64 phys_hsa;
+    u32 phys_hsa_lo, phys_hsa_hi;   
+    uint64_t phys_hsa, msr_content;
     int rc, cpu = smp_processor_id();
     struct cpuinfo_x86 *c = &cpu_data[cpu];
  
     /* Check whether SVM feature is disabled in BIOS */
-    rdmsr(MSR_K8_VM_CR, eax, edx);
-    if ( eax & K8_VMCR_SVME_DISABLE )
+    rdmsrl(MSR_K8_VM_CR, msr_content);
+    if ( msr_content & K8_VMCR_SVME_DISABLE )
     {
         printk("CPU%d: AMD SVM Extension is disabled in BIOS.\n", cpu);
         return -EINVAL;
@@ -892,15 +892,14 @@ static int svm_cpu_up(void)
      * Check whether EFER.LMSLE can be written.
      * Unfortunately there's no feature bit defined for this.
      */
-    eax = read_efer();
-    edx = read_efer() >> 32;
-    if ( wrmsr_safe(MSR_EFER, eax | EFER_LMSLE, edx) == 0 )
-        rdmsr(MSR_EFER, eax, edx);
-    if ( eax & EFER_LMSLE )
+    msr_content = read_efer();
+    if ( wrmsr_safe(MSR_EFER, msr_content | EFER_LMSLE) == 0 )
+        rdmsrl(MSR_EFER, msr_content);
+    if ( msr_content & EFER_LMSLE )
     {
         if ( c == &boot_cpu_data )
             cpu_has_lmsl = 1;
-        wrmsr(MSR_EFER, eax ^ EFER_LMSLE, edx);
+        wrmsrl(MSR_EFER, msr_content ^ EFER_LMSLE);
     }
     else
     {
@@ -1033,7 +1032,6 @@ static void svm_dr_access(struct vcpu *v
 
 static int svm_msr_read_intercept(unsigned int msr, uint64_t *msr_content)
 {
-    u32 eax, edx;
     struct vcpu *v = current;
     struct vmcb_struct *vmcb = v->arch.hvm_svm.vmcb;
 
@@ -1111,11 +1109,8 @@ static int svm_msr_read_intercept(unsign
              rdmsr_hypervisor_regs(msr, msr_content) )
             break;
 
-        if ( rdmsr_safe(msr, eax, edx) == 0 )
-        {
-            *msr_content = ((uint64_t)edx << 32) | eax;
+        if ( rdmsr_safe(msr, *msr_content) == 0 )
             break;
-        }
 
         goto gpf;
     }
xen/arch/x86/hvm/vmx/vmx.c +1 -6
diff -r 04e81a65d27e xen/arch/x86/hvm/vmx/vmx.c
@@ -1809,8 +1809,6 @@ static int is_last_branch_msr(u32 ecx)
 
 static int vmx_msr_read_intercept(unsigned int msr, uint64_t *msr_content)
 {
-    u32 eax, edx;
-
     HVM_DBG_LOG(DBG_LEVEL_1, "ecx=%x", msr);
 
     switch ( msr )
@@ -1866,11 +1864,8 @@ static int vmx_msr_read_intercept(unsign
              rdmsr_hypervisor_regs(msr, msr_content) )
             break;
 
-        if ( rdmsr_safe(msr, eax, edx) == 0 )
-        {
-            *msr_content = ((uint64_t)edx << 32) | eax;
+        if ( rdmsr_safe(msr, *msr_content) == 0 )
             break;
-        }
 
         goto gp_fault;
     }
xen/arch/x86/traps.c +35 -30

160 lines not on this page · raw message

xen/arch/x86/x86_64/mm.c +3 -3
diff -r 04e81a65d27e xen/arch/x86/x86_64/mm.c
@@ -1079,21 +1079,21 @@ long do_set_segment_base(unsigned int wh
     switch ( which )
     {
     case SEGBASE_FS:
-        if ( wrmsr_safe(MSR_FS_BASE, base, base>>32) )
+        if ( wrmsr_safe(MSR_FS_BASE, base) )
             ret = -EFAULT;
         else
             v->arch.guest_context.fs_base = base;
         break;
 
     case SEGBASE_GS_USER:
-        if ( wrmsr_safe(MSR_SHADOW_GS_BASE, base, base>>32) )
+        if ( wrmsr_safe(MSR_SHADOW_GS_BASE, base) )
             ret = -EFAULT;
         else
             v->arch.guest_context.gs_base_user = base;
         break;
 
     case SEGBASE_GS_KERNEL:
-        if ( wrmsr_safe(MSR_GS_BASE, base, base>>32) )
+        if ( wrmsr_safe(MSR_GS_BASE, base) )
             ret = -EFAULT;
         else
             v->arch.guest_context.gs_base_kernel = base;
xen/arch/x86/x86_64/mmconfig-shared.c +7 -10
diff -r 04e81a65d27e xen/arch/x86/x86_64/mmconfig-shared.c
@@ -125,8 +125,8 @@ static const char __init *pci_mmcfg_inte
 
 static const char __init *pci_mmcfg_amd_fam10h(void)
 {
-    u32 low, high, address;
-    u64 base, msr;
+    uint32_t address;
+    uint64_t base, msr_content;
     int i;
     unsigned segnbits = 0, busnbits;
 
@@ -134,20 +134,17 @@ static const char __init *pci_mmcfg_amd_
         return NULL;
 
     address = MSR_FAM10H_MMIO_CONF_BASE;
-    if (rdmsr_safe(address, low, high))
+    if (rdmsr_safe(address, msr_content))
         return NULL;
 
-    msr = high;
-    msr <<= 32;
-    msr |= low;
-
     /* mmconfig is not enable */
-    if (!(msr & FAM10H_MMIO_CONF_ENABLE))
+    if (!(msr_content & FAM10H_MMIO_CONF_ENABLE))
         return NULL;
 
-    base = msr & (FAM10H_MMIO_CONF_BASE_MASK<<FAM10H_MMIO_CONF_BASE_SHIFT);
+    base = msr_content &
+        (FAM10H_MMIO_CONF_BASE_MASK<<FAM10H_MMIO_CONF_BASE_SHIFT);
 
-    busnbits = (msr >> FAM10H_MMIO_CONF_BUSRANGE_SHIFT) &
+    busnbits = (msr_content >> FAM10H_MMIO_CONF_BUSRANGE_SHIFT) &
                 FAM10H_MMIO_CONF_BUSRANGE_MASK;
 
     /*
xen/include/asm-x86/msr.h +25 -15
diff -r 04e81a65d27e xen/include/asm-x86/msr.h
@@ -6,7 +6,9 @@
 #ifndef __ASSEMBLY__
 
 #include <xen/smp.h>
+#include <xen/types.h>
 #include <xen/percpu.h>
+#include <xen/errno.h>
 
 #define rdmsr(msr,val1,val2) \
      __asm__ __volatile__("rdmsr" \
@@ -34,8 +36,9 @@ static inline void wrmsrl(unsigned int m
 }
 
 /* rdmsr with exception handling */
-#define rdmsr_safe(msr,val1,val2) ({\
+#define rdmsr_safe(msr,val) ({\
     int _rc; \
+    uint32_t val1, val2; \
     __asm__ __volatile__( \
         "1: rdmsr\n2:\n" \
         ".section .fixup,\"ax\"\n" \
@@ -47,23 +50,30 @@ static inline void wrmsrl(unsigned int m
         ".previous\n" \
         : "=a" (val1), "=d" (val2), "=&r" (_rc) \
         : "c" (msr), "2" (0), "i" (-EFAULT)); \
+    val = val2 | ((uint64_t)val1 << 32); \
     _rc; })
 
 /* wrmsr with exception handling */
-#define wrmsr_safe(msr,val1,val2) ({\
-    int _rc; \
-    __asm__ __volatile__( \
-        "1: wrmsr\n2:\n" \
-        ".section .fixup,\"ax\"\n" \
-        "3: movl %5,%0\n; jmp 2b\n" \
-        ".previous\n" \
-        ".section __ex_table,\"a\"\n" \
-        "   "__FIXUP_ALIGN"\n" \
-        "   "__FIXUP_WORD" 1b,3b\n" \
-        ".previous\n" \
-        : "=&r" (_rc) \
-        : "c" (msr), "a" (val1), "d" (val2), "0" (0), "i" (-EFAULT)); \
-    _rc; })
+static inline int wrmsr_safe(unsigned int msr, uint64_t val)
+{
+    int _rc;
+    uint32_t lo, hi;
+    lo = (uint32_t)val;
+    hi = (uint32_t)(val >> 32);
+
+    __asm__ __volatile__(
+        "1: wrmsr\n2:\n"
+        ".section .fixup,\"ax\"\n"
+        "3: movl %5,%0\n; jmp 2b\n"
+        ".previous\n"
+        ".section __ex_table,\"a\"\n"
+        "   "__FIXUP_ALIGN"\n"
+        "   "__FIXUP_WORD" 1b,3b\n"
+        ".previous\n"
+        : "=&r" (_rc)
+        : "c" (msr), "a" (lo), "d" (hi), "0" (0), "i" (-EFAULT));
+    return _rc;
+}
 
 #define rdtsc(low,high) \
      __asm__ __volatile__("rdtsc" : "=a" (low), "=d" (high))

_______________________________________________
Xen-devel mailing list
[email protected]
http://lists.xensource.com/xen-devel

#1 lore raw

xen/arch/x86/cpu/amd.c int force_mwait __cpuinitdata; · line 249

Quoted 10 lines · Christoph Egger
xen/arch/x86/cpu/amd.c +5 -5
-	if ((rdmsr_safe(MSR_K8_ENABLE_C1E, lo, hi) == 0) &&
-	    (lo & (3u << 27)) &&
-	    (wrmsr_safe(MSR_K8_ENABLE_C1E, lo & ~(3u << 27), hi) != 0))
-		printk(KERN_ERR "Failed to disable C1E on CPU#%u (%08x)\n",
-		       smp_processor_id(), lo);
+	if ((rdmsr_safe(MSR_K8_ENABLE_C1E, msr_content) == 0) &&
+	    (msr_content & (3u << 27)) &&
+	    (wrmsr_safe(MSR_K8_ENABLE_C1E, msr_content & ~(3u << 27)) != 0))
+		printk(KERN_ERR "Failed to disable C1E on CPU#%u (%16"PRIx64")\n",
+		       smp_processor_id(), msr_content);

Broken. Please use ull instead of just u in your masks, especially the last one.

xen/arch/x86/cpu/mtrr/generic.c void __init mtrr_state_warn(void) · line 116

Quoted 10 lines

void mtrr_wrmsr(unsigned msr, unsigned a, unsigned b) {

xen/arch/x86/cpu/mtrr/generic.c +4 -1
-	if (wrmsr_safe(msr, a, b) < 0)
+	uint64_t msr_content;
+
+	msr_content = ((uint64_t)b << 32) | a;
+	if (wrmsr_safe(msr, msr_content) < 0)
 		printk(KERN_ERR
 			"MTRR: CPU %u: Writing MSR %x to %x:%x failed\n",
 			smp_processor_id(), msr, a, b);

Since you keep the two unsigned parameters of the function this change complicates the code more than it cleans up.

Joerg

#2 lore raw
Quoted 14 lines · On Friday 11 June 2010 14:23:58 Joerg Roedel wrote:

Review

Quoted 10 lines · Christoph Egger
(quoted diff) +5 -5
-	if ((rdmsr_safe(MSR_K8_ENABLE_C1E, lo, hi) == 0) &&
-	    (lo & (3u << 27)) &&
-	    (wrmsr_safe(MSR_K8_ENABLE_C1E, lo & ~(3u << 27), hi) != 0))
-		printk(KERN_ERR "Failed to disable C1E on CPU#%u (%08x)\n",
-		       smp_processor_id(), lo);
+	if ((rdmsr_safe(MSR_K8_ENABLE_C1E, msr_content) == 0) &&
+	    (msr_content & (3u << 27)) &&
+	    (wrmsr_safe(MSR_K8_ENABLE_C1E, msr_content & ~(3u << 27)) != 0))
+		printk(KERN_ERR "Failed to disable C1E on CPU#%u (%16"PRIx64")\n",
+		       smp_processor_id(), msr_content);

Broken. Please use ull instead of just u in your masks, especially the last one.

Thanks for pointing this out. Attached patch fixes this.

Quoted 14 lines

Review

Quoted 11 lines

void mtrr_wrmsr(unsigned msr, unsigned a, unsigned b) {

(quoted diff) +4 -1
-	if (wrmsr_safe(msr, a, b) < 0)
+	uint64_t msr_content;
+
+	msr_content = ((uint64_t)b << 32) | a;
+	if (wrmsr_safe(msr, msr_content) < 0)
 		printk(KERN_ERR
 			"MTRR: CPU %u: Writing MSR %x to %x:%x failed\n",
 			smp_processor_id(), msr, a, b);

Since you keep the two unsigned parameters of the function this change complicates the code more than it cleans up.

Fixed in c/s 21603.

Christoph

Signature · 6 lines
---to satisfy European Law for business letters:
Advanced Micro Devices GmbH
Einsteinring 24, 85609 Dornach b. Muenchen
Geschaeftsfuehrer: Andrew Bowd, Thomas M. McCoy, Giuliano Meroni
Sitz: Dornach, Gemeinde Aschheim, Landkreis Muenchen
Registergericht Muenchen, HRB Nr. 43632
xen/arch/x86/cpu/amd.c +2 -2
diff -r 9f49e9794596 xen/arch/x86/cpu/amd.c
@@ -243,8 +243,8 @@ static void disable_c1e(void *unused)
 	 * but we safely catch the #GP in that case.
 	 */
 	if ((rdmsr_safe(MSR_K8_ENABLE_C1E, msr_content) == 0) &&
-	    (msr_content & (3u << 27)) &&
-	    (wrmsr_safe(MSR_K8_ENABLE_C1E, msr_content & ~(3u << 27)) != 0))
+	    (msr_content & (3ULL << 27)) &&
+	    (wrmsr_safe(MSR_K8_ENABLE_C1E, msr_content & ~(3ULL << 27)) != 0))
 		printk(KERN_ERR "Failed to disable C1E on CPU#%u (%16"PRIx64")\n",
 		       smp_processor_id(), msr_content);
 }

_______________________________________________
Xen-devel mailing list
[email protected]
http://lists.xensource.com/xen-devel