xen: msr safe cleanup
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
@@ -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
@@ -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
@@ -857,14 +857,14 @@ static void svm_init_erratum_383(structstatic 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 *vstatic 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
@@ -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
@@ -1079,21 +1079,21 @@ long do_set_segment_base(unsigned int whswitch ( 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
@@ -125,8 +125,8 @@ static const char __init *pci_mmcfg_intestatic 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
@@ -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
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
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
@@ -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