From mboxrd@z Thu Jan  1 00:00:00 1970
From: Joerg Roedel <joro@8bytes.org>
Subject: Re: [PATCH] xen: msr safe cleanup
Date: Fri, 11 Jun 2010 14:23:58 +0200
Message-ID: <20100611122358.GB8302@8bytes.org>
References: <201006111211.21822.Christoph.Egger@amd.com>
Mime-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Return-path: <xen-devel-bounces@lists.xensource.com>
Content-Disposition: inline
In-Reply-To: <201006111211.21822.Christoph.Egger@amd.com>
List-Unsubscribe: <http://lists.xensource.com/mailman/listinfo/xen-devel>,
	<mailto:xen-devel-request@lists.xensource.com?subject=unsubscribe>
List-Post: <mailto:xen-devel@lists.xensource.com>
List-Help: <mailto:xen-devel-request@lists.xensource.com?subject=help>
List-Subscribe: <http://lists.xensource.com/mailman/listinfo/xen-devel>,
	<mailto:xen-devel-request@lists.xensource.com?subject=subscribe>
Sender: xen-devel-bounces@lists.xensource.com
Errors-To: xen-devel-bounces@lists.xensource.com
To: Christoph Egger <Christoph.Egger@amd.com>
Cc: xen-devel@lists.xensource.com
List-Id: xen-devel@lists.xenproject.org

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