From mboxrd@z Thu Jan 1 00:00:00 1970 From: Christoph Egger Subject: Re: [PATCH] xen: msr safe cleanup Date: Fri, 11 Jun 2010 17:48:27 +0200 Message-ID: <201006111748.27904.Christoph.Egger@amd.com> References: <201006111211.21822.Christoph.Egger@amd.com> <20100611122358.GB8302@8bytes.org> Mime-Version: 1.0 Content-Type: multipart/mixed; boundary="Boundary-00=_LrlEM3wMrHkWeVU" Return-path: In-Reply-To: <20100611122358.GB8302@8bytes.org> List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xensource.com Errors-To: xen-devel-bounces@lists.xensource.com To: xen-devel@lists.xensource.com Cc: Joerg Roedel List-Id: xen-devel@lists.xenproject.org --Boundary-00=_LrlEM3wMrHkWeVU Content-Type: text/plain; charset="iso-8859-1" Content-Transfer-Encoding: 7bit Content-Disposition: inline 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 > > > 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 --Boundary-00=_LrlEM3wMrHkWeVU Content-Type: text/x-diff; charset="iso 8859-15"; name="xen_msrsafe_fix.diff" Content-Transfer-Encoding: 7bit Content-Disposition: attachment; filename="xen_msrsafe_fix.diff" 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); } --Boundary-00=_LrlEM3wMrHkWeVU Content-Type: text/plain; charset="us-ascii" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit Content-Disposition: inline _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel --Boundary-00=_LrlEM3wMrHkWeVU--