Re: [Xen-devel] [PATCH] xen: msr safe cleanup

[Joerg Roedel <joro@xxxxxxxxxx>]

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@xxxxxxxxxxxxxxxxxxx
http://lists.xensource.com/xen-devel