Re: [PATCH] xen: msr safe cleanup

[Christoph Egger <Christoph.Egger@amd.com>]

[-- Attachment #1: Type: text/plain, Size: 1676 bytes --]

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

[-- Attachment #2: xen_msrsafe_fix.diff --]
[-- Type: text/x-diff, Size: 669 bytes --]

diff -r 9f49e9794596 xen/arch/x86/cpu/amd.c
--- a/xen/arch/x86/cpu/amd.c	Fri Jun 11 15:22:12 2010 +0100
+++ b/xen/arch/x86/cpu/amd.c	Fri Jun 11 17:44:41 2010 +0200
@@ -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);
 }

[-- Attachment #3: Type: text/plain, Size: 138 bytes --]

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel