Hi! Attached patch cleans up msr-handling with rdmsr_safe/wrmsr_safe. This is part is extracted from the big msr cleanup patch. Signed-off-by: Christoph Egger <Christoph.Egger@amd.com> -- ---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-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
On Fri, Jun 11, 2010 at 12:11:21PM +0200, Christoph Egger wrote:> - 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.> 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);Since you keep the two unsigned parameters of the function this change complicates the code more than it cleans up. Joerg _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
On Friday 11 June 2010 14:23:58 Joerg Roedel wrote:> On Fri, Jun 11, 2010 at 12:11:21PM +0200, Christoph Egger wrote: > > - 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. Signed-off-by: Christoph Egger <Christoph.Egger@amd.com>> > > 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); > > 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 -- ---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-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel