kernel/microcode.c error from new 64bit code

kernel/microcode.c error from new 64bit code

[Stephen Hemminger]

In the mad rush to put in Intel 64 bit support, did anyone make sure and not
break the 32 bit build?

arch/i386/kernel/microcode.c: In function `do_update_one':
arch/i386/kernel/microcode.c:374: warning: cast from pointer to integer of different size
arch/i386/kernel/microcode.c:374: warning: cast from pointer to integer of different size
arch/i386/kernel/microcode.c:374: error: impossible register constraint in `asm'
arch/i386/kernel/microcode.c:389: confused by earlier errors, bailing out
make[1]: *** [arch/i386/kernel/microcode.o] Error 1
make[1]: Target `__build' not remade because of errors.
make: *** [arch/i386/kernel] Error 2

Re: kernel/microcode.c error from new 64bit code

[Linus Torvalds]

On Wed, 18 Feb 2004, Stephen Hemminger wrote:
>
> In the mad rush to put in Intel 64 bit support, did anyone make sure and not
> break the 32 bit build?

Heh. Somebody has the driver enabled ;).

How about this patch?

		Linus

---
===== arch/i386/kernel/microcode.c 1.24 vs edited =====
--- 1.24/arch/i386/kernel/microcode.c	Tue Feb 17 18:14:37 2004
+++ edited/arch/i386/kernel/microcode.c	Wed Feb 18 15:05:38 2004
@@ -371,8 +371,9 @@
 	spin_lock_irqsave(&microcode_update_lock, flags);          
 
 	/* write microcode via MSR 0x79 */
-	wrmsr(MSR_IA32_UCODE_WRITE, (u64)(uci->mc->bits), 
-	      (u64)(uci->mc->bits) >> 32);
+	wrmsr(MSR_IA32_UCODE_WRITE,
+		(unsigned long) uci->mc->bits, 
+		(unsigned long) uci->mc->bits >> 16 >> 16);
 	wrmsr(MSR_IA32_UCODE_REV, 0, 0);
 
 	__asm__ __volatile__ ("cpuid" : : : "ax", "bx", "cx", "dx");

Re: kernel/microcode.c error from new 64bit code

[Linus Torvalds]

On Wed, 18 Feb 2004, Linus Torvalds wrote:
> 
> How about this patch?

Btw, it does show a bug in our "wrmsr()" macro on x86.

We should make sure to cast the values to 32-bit values in wrmsr(), or use
an inline function to make sure that the right conversions happen. Because
we arguably shouldn't give some meaningless error message just because we
passed in a 64-bit argument.

So in addition to fixing "microcode.c" to not do the silly thing in the 
first place, we should probably clean up wrmsr().

I'll think about it. In one sense it was _useful_ to see that the 
microcode update tried to do something that was nonsensical on a 32-bit 
x86. On the other hand, the compiler should do the right thing regardless, 
and it might be useful to allow 64-bit values to be silently truncated 
for compatibility reasons.

I'll commit the microcode fix, and think a bit more about the wrmsr() 
thing.

		Linus

Re: kernel/microcode.c error from new 64bit code

[Pavel Machek]

Hi!

> > In the mad rush to put in Intel 64 bit support, did anyone make sure and not
> > break the 32 bit build?
> 
> Heh. Somebody has the driver enabled ;).
> 
> How about this patch?
> 
> 		Linus
> 
> ---
> ===== arch/i386/kernel/microcode.c 1.24 vs edited =====
> --- 1.24/arch/i386/kernel/microcode.c	Tue Feb 17 18:14:37 2004
> +++ edited/arch/i386/kernel/microcode.c	Wed Feb 18 15:05:38 2004
> @@ -371,8 +371,9 @@
>  	spin_lock_irqsave(&microcode_update_lock, flags);          
>  
>  	/* write microcode via MSR 0x79 */
> -	wrmsr(MSR_IA32_UCODE_WRITE, (u64)(uci->mc->bits), 
> -	      (u64)(uci->mc->bits) >> 32);
> +	wrmsr(MSR_IA32_UCODE_WRITE,
> +		(unsigned long) uci->mc->bits, 
> +		(unsigned long) uci->mc->bits >> 16 >> 16);
				             ~~~~~~~~~~~~

I see what you are doing, but this is evil. At least comment /* ">> 32"
is undefined on i386 */ ?
								Pavel

-- 
When do you have a heart between your knees?
[Johanka's followup: and *two* hearts?]

Re: kernel/microcode.c error from new 64bit code

[Linus Torvalds]

On Sat, 21 Feb 2004, Pavel Machek wrote:
>
> > +	wrmsr(MSR_IA32_UCODE_WRITE,
> > +		(unsigned long) uci->mc->bits, 
> > +		(unsigned long) uci->mc->bits >> 16 >> 16);
> 				             ~~~~~~~~~~~~
> 
> I see what you are doing, but this is evil. At least comment /* ">> 32"
> is undefined on i386 */ ?

Sorry, but you're wrong.

">> 32" is underfined PERIOD! It has nothing to do with x86, it's a C
standards issue. It's undefined on any 32-bit architecture. (shifting by
the wordsize or bigger is simply not a defined C operation).

The above is not evil. The above is the standard way of doing this in C if 
you know the word-size is 32-bits or bigger.

		Linus

Re: kernel/microcode.c error from new 64bit code

[Pavel Machek]

Hi!

> > > +	wrmsr(MSR_IA32_UCODE_WRITE,
> > > +		(unsigned long) uci->mc->bits, 
> > > +		(unsigned long) uci->mc->bits >> 16 >> 16);
> > 				             ~~~~~~~~~~~~
> > 
> > I see what you are doing, but this is evil. At least comment /* ">> 32"
> > is undefined on i386 */ ?
> 
> Sorry, but you're wrong.
> 
> ">> 32" is underfined PERIOD! It has nothing to do with x86, it's a C
> standards issue. It's undefined on any 32-bit architecture. (shifting by
> the wordsize or bigger is simply not a defined C operation).

Yep, that is what I wanted to say. [This driver only has meaning for
i386 and ia32e => if we have 32-bit architecture, it must be i386.]

> The above is not evil. The above is the standard way of doing this in C if 
> you know the word-size is 32-bits or bigger.

I'm just afraid that someone will mail you a patch replacing that with
>> 32 and you'll overlook it.

								Pavel
-- 
When do you have a heart between your knees?
[Johanka's followup: and *two* hearts?]

Re: kernel/microcode.c error from new 64bit code

[Linus Torvalds]

On Sat, 21 Feb 2004, Pavel Machek wrote:
>
> I'm just afraid that someone will mail you a patch replacing that with
> >> 32 and you'll overlook it.

Well, the good news is that ">> 32" should cause gcc to complain with a 
big warning (exactly because it's undefined brhaviour on a 32-bit 
architecture), so I don't think it's easy to overlook.

		Linus

Re: kernel/microcode.c error from new 64bit code

[Eric W. Biederman]

Linus Torvalds <torvalds@osdl.org> writes:

> On Sat, 21 Feb 2004, Pavel Machek wrote:
> >
> > I'm just afraid that someone will mail you a patch replacing that with
> > >> 32 and you'll overlook it.
> 
> Well, the good news is that ">> 32" should cause gcc to complain with a 
> big warning (exactly because it's undefined brhaviour on a 32-bit 
> architecture), so I don't think it's easy to overlook.

What is wrong with the original?

-       wrmsr(MSR_IA32_UCODE_WRITE, (unsigned int)(uci->mc->bits), 0);

I don't see how anything else could be correct.

Either we have high bits we need to worry about in 32bit mode, in which
case the 32bit variant is wrong.  Or we don't have high bits to worry
about in which case attempting to set them is wrong.

Eric

Re: kernel/microcode.c error from new 64bit code

[Pavel Machek]

Hi!

> > On Sat, 21 Feb 2004, Pavel Machek wrote:
> > >
> > > I'm just afraid that someone will mail you a patch replacing that with
> > > >> 32 and you'll overlook it.
> > 
> > Well, the good news is that ">> 32" should cause gcc to complain with a 
> > big warning (exactly because it's undefined brhaviour on a 32-bit 
> > architecture), so I don't think it's easy to overlook.
> 
> What is wrong with the original?
> 
> -       wrmsr(MSR_IA32_UCODE_WRITE, (unsigned int)(uci->mc->bits), 0);
> 
> I don't see how anything else could be correct.
> 
> Either we have high bits we need to worry about in 32bit mode, in which
> case the 32bit variant is wrong.  Or we don't have high bits to worry
> about in which case attempting to set them is wrong.

I believe that driver is now shared between i386 and x86-64.

								Pavel
-- 
When do you have a heart between your knees?
[Johanka's followup: and *two* hearts?]

Re: kernel/microcode.c error from new 64bit code

[Linus Torvalds]

On Sat, 21 Feb 2004, Eric W. Biederman wrote:
> 
> What is wrong with the original?
> 
> -       wrmsr(MSR_IA32_UCODE_WRITE, (unsigned int)(uci->mc->bits), 0);
> 
> I don't see how anything else could be correct.

Did you miss the fact that we now support x86-64, and that if compiled 
that way the address is 64-bit?

		Linus

Re: kernel/microcode.c error from new 64bit code

[Eric W. Biederman]

Linus Torvalds <torvalds@osdl.org> writes:

> On Sat, 21 Feb 2004, Eric W. Biederman wrote:
> > 
> > What is wrong with the original?
> > 
> > -       wrmsr(MSR_IA32_UCODE_WRITE, (unsigned int)(uci->mc->bits), 0);
> > 
> > I don't see how anything else could be correct.
> 
> Did you miss the fact that we now support x86-64, and that if compiled 
> that way the address is 64-bit?

No I missed the fact that uci->mc->bits was an address.  Now it
makes sense how we can have a 64bit value only in 64bit mode....

The code still feels a little awkward but after looking at it
some more I don't see any problems.

Eric

Re: kernel/microcode.c error from new 64bit code

[H. Peter Anvin]

Followup to:  <Pine.LNX.4.58.0402210914530.3301@ppc970.osdl.org>
By author:    Linus Torvalds <torvalds@osdl.org>
In newsgroup: linux.dev.kernel
>
> 
> 
> On Sat, 21 Feb 2004, Pavel Machek wrote:
> >
> > > +	wrmsr(MSR_IA32_UCODE_WRITE,
> > > +		(unsigned long) uci->mc->bits, 
> > > +		(unsigned long) uci->mc->bits >> 16 >> 16);
> > 				             ~~~~~~~~~~~~
> > 
> > I see what you are doing, but this is evil. At least comment /* ">> 32"
> > is undefined on i386 */ ?
> 
> Sorry, but you're wrong.
> 
> ">> 32" is underfined PERIOD! It has nothing to do with x86, it's a C
> standards issue. It's undefined on any 32-bit architecture. (shifting by
> the wordsize or bigger is simply not a defined C operation).
> 
> The above is not evil. The above is the standard way of doing this in C if 
> you know the word-size is 32-bits or bigger.
> 

Actually, what is undefined is shifting with the size of the data type or higher.

If the cast of uci->mc->bits had been an uint64_t (unsigned long
long), >> 32 would have been perfectly well-defined.

The above code is just plain wrong: the cast to (unsigned long) has
higher precedence than the shift, so on i386 (which I presume this is)
it will become an unsigned long, and the shifts will bring it down to
zero.

You might as well write zero if that's what you mean.

	-hpa

Re: kernel/microcode.c error from new 64bit code

[Linus Torvalds]

On Sun, 22 Feb 2004, H. Peter Anvin wrote:
> 
> The above code is just plain wrong: the cast to (unsigned long) has
> higher precedence than the shift, so on i386 (which I presume this is)
> it will become an unsigned long, and the shifts will bring it down to
> zero.

WHICH IS WHAT WE WANT. On x86.

But ..

> You might as well write zero if that's what you mean.

No. Because on x86-64 it is NOT zero. Because there "unsigned long" is
64-bit, and it results in the high 32 bits. Which is, again, exactly what
we want.

Guys, give it up. The code is not only already committed, it's simply the 
best way to do what it does.

		Linus

Re: kernel/microcode.c error from new 64bit code

[H. Peter Anvin]

Linus Torvalds wrote:
> 
> No. Because on x86-64 it is NOT zero. Because there "unsigned long" is
> 64-bit, and it results in the high 32 bits. Which is, again, exactly what
> we want.
> 

Ah yes, dual-mode code.  Should have figured.

> Guys, give it up. The code is not only already committed, it's simply the 
> best way to do what it does.

Perhaps an even better thing to have would be a wrmsr64() and rdmsr64() 
routines, for the MSRs which genuinely are a 64-bit item.  Splitting 
them up is rather ugly when it's a real 64-bit value.

Then the code would just be:

	/* 32 bits on x64, 64 bits on x86-64 */
	wrmsr64(MSR_NUMBER, (unsigned long)value);

I think the comment would be justified.

If you agree I'll send a patch.

	-hpa

Re: kernel/microcode.c error from new 64bit code

[H. Peter Anvin]

It turns out wrmsr64() and rdmsr64() is already in the code, except 
they're called wrmsrl() and rdmsrl().

So I'd suggest the following:

Index: microcode.c
===================================================================
RCS file: /home/hpa/kernel/bkcvs/linux-2.5/arch/i386/kernel/microcode.c,v
retrieving revision 1.27
diff -u -r1.27 microcode.c
--- microcode.c 19 Feb 2004 04:48:45 -0000      1.27
+++ microcode.c 22 Feb 2004 20:40:30 -0000
@@ -371,9 +371,8 @@
         spin_lock_irqsave(&microcode_update_lock, flags);

         /* write microcode via MSR 0x79 */
-       wrmsr(MSR_IA32_UCODE_WRITE,
-               (unsigned long) uci->mc->bits,
-               (unsigned long) uci->mc->bits >> 16 >> 16);
+       /* Note: unsigned long is 32 bits on i386, 64 bits on x86-64 */
+       wrmsrl(MSR_IA32_UCODE_WRITE, (unsigned long) uci->mc->bits);
         wrmsr(MSR_IA32_UCODE_REV, 0, 0);

         __asm__ __volatile__ ("cpuid" : : : "ax", "bx", "cx", "dx");

RE: kernel/microcode.c error from new 64bit code

[Nakajima, Jun]

Looks good to me.

Jun
>-----Original Message-----
>From: linux-kernel-owner@vger.kernel.org [mailto:linux-kernel-
>owner@vger.kernel.org] On Behalf Of Linus Torvalds
>Sent: Wednesday, February 18, 2004 3:08 PM
>To: Stephen Hemminger
>Cc: ak@suse.de; linux-kernel@vger.kernel.org
>Subject: Re: kernel/microcode.c error from new 64bit code
>
>
>
>On Wed, 18 Feb 2004, Stephen Hemminger wrote:
>>
>> In the mad rush to put in Intel 64 bit support, did anyone make sure
and
>not
>> break the 32 bit build?
>
>Heh. Somebody has the driver enabled ;).
>
>How about this patch?
>
>		Linus
>
>---
>===== arch/i386/kernel/microcode.c 1.24 vs edited =====
>--- 1.24/arch/i386/kernel/microcode.c	Tue Feb 17 18:14:37 2004
>+++ edited/arch/i386/kernel/microcode.c	Wed Feb 18 15:05:38 2004
>@@ -371,8 +371,9 @@
> 	spin_lock_irqsave(&microcode_update_lock, flags);
>
> 	/* write microcode via MSR 0x79 */
>-	wrmsr(MSR_IA32_UCODE_WRITE, (u64)(uci->mc->bits),
>-	      (u64)(uci->mc->bits) >> 32);
>+	wrmsr(MSR_IA32_UCODE_WRITE,
>+		(unsigned long) uci->mc->bits,
>+		(unsigned long) uci->mc->bits >> 16 >> 16);
> 	wrmsr(MSR_IA32_UCODE_REV, 0, 0);
>
> 	__asm__ __volatile__ ("cpuid" : : : "ax", "bx", "cx", "dx");
>-
>To unsubscribe from this list: send the line "unsubscribe linux-kernel"
in
>the body of a message to majordomo@vger.kernel.org
>More majordomo info at  http://vger.kernel.org/majordomo-info.html
>Please read the FAQ at  http://www.tux.org/lkml/