Re: [LinuxBIOS] Intel microcode revision code

On Dec 12, 2007 12:28 AM, <Martin.Karlsson@emerson.com> wrote:
Hello,
Yesterday I wrote some code for the Syslinux boot loader that is meant to boot different images based on which revision of the CPU microcode that is currently loaded in the CPU (yes, this is a bit strange but we have a very good reason for it). I then compared my implementation with a few others an ran across yours, in http://www.openbios.org/viewvc/trunk/LinuxBIOSv2/src/cpu/intel/microcode/mic...
Question to you guys: why is the first wrmsr instruction there? From my understanding, by not properly initialising ECX, EAX and EDX this will overwrite whatever is in the MSR pointed to by ECX?!
BTW I tried out your code on our target hardware (Intel Celeron M, 600 MHz) and with that first wrmsr line in place it hangs and without it, it runs just fine.
Thanks Martin. That looks like quite a nice bug catch you've done :-) We'll have to figure out if this code was ever (a) used and (b) worked :-) Thanks again, this is great! ron

* ron minnich <rminnich@gmail.com> [071212 17:19]:
Question to you guys: why is the first wrmsr instruction there? From my understanding, by not properly initialising ECX, EAX and EDX this will overwrite whatever is in the MSR pointed to by ECX?!
BTW I tried out your code on our target hardware (Intel Celeron M, 600 MHz) and with that first wrmsr line in place it hangs and without it, it runs just fine.
Thanks Martin. That looks like quite a nice bug catch you've done :-)
Here's a patch that resolves the issue. -- coresystems GmbH • Brahmsstr. 16 • D-79104 Freiburg i. Br. Tel.: +49 761 7668825 • Fax: +49 761 7664613 Email: info@coresystems.de • http://www.coresystems.de/ Registergericht: Amtsgericht Freiburg • HRB 7656 Geschäftsführer: Stefan Reinauer • Ust-IdNr.: DE245674866

On 20.02.2008 17:19, Stefan Reinauer wrote:
* ron minnich <rminnich@gmail.com> [071212 17:19]:
Question to you guys: why is the first wrmsr instruction there? From my understanding, by not properly initialising ECX, EAX and EDX this will overwrite whatever is in the MSR pointed to by ECX?!
BTW I tried out your code on our target hardware (Intel Celeron M, 600 MHz) and with that first wrmsr line in place it hangs and without it, it runs just fine.
Thanks Martin. That looks like quite a nice bug catch you've done :-)
Here's a patch that resolves the issue.
Signed-off-by: Stefan Reinauer <stepan@coresystems.de>
Index: src/cpu/intel/microcode/microcode.c =================================================================== --- src/cpu/intel/microcode/microcode.c (revision 3111) +++ src/cpu/intel/microcode/microcode.c (working copy) @@ -33,7 +33,6 @@ */ msr_t msr; __asm__ volatile ( - "wrmsr\n\t"
ACK.
"xorl %%eax, %%eax\n\t" "xorl %%edx, %%edx\n\t" "movl $0x8b, %%ecx\n\t" @@ -60,7 +59,7 @@ char *c; msr_t msr;
- /* cpuid sets msr 0x8B iff a microcode update has been loaded. */ + /* cpuid sets msr 0x8B if a microcode update has been loaded. */
NACK. "IFF" is shorthand for "if and only if", see http://en.wikipedia.org/wiki/If_and_only_if
msr.lo = 0; msr.hi = 0; wrmsr(0x8B, msr);
Regards, Carl-Daniel -- http://www.hailfinger.org/

Carl-Daniel Hailfinger wrote:
On 20.02.2008 17:19, Stefan Reinauer wrote:
* ron minnich <rminnich@gmail.com> [071212 17:19]:
Question to you guys: why is the first wrmsr instruction there? From my understanding, by not properly initialising ECX, EAX and EDX this will overwrite whatever is in the MSR pointed to by ECX?!
BTW I tried out your code on our target hardware (Intel Celeron M, 600 MHz) and with that first wrmsr line in place it hangs and without it, it runs just fine.
Thanks Martin. That looks like quite a nice bug catch you've done :-)
Here's a patch that resolves the issue.
Signed-off-by: Stefan Reinauer <stepan@coresystems.de>
Index: src/cpu/intel/microcode/microcode.c =================================================================== --- src/cpu/intel/microcode/microcode.c (revision 3111) +++ src/cpu/intel/microcode/microcode.c (working copy) @@ -33,7 +33,6 @@ */ msr_t msr; __asm__ volatile ( - "wrmsr\n\t"
ACK.
"xorl %%eax, %%eax\n\t" "xorl %%edx, %%edx\n\t" "movl $0x8b, %%ecx\n\t" @@ -60,7 +59,7 @@ char *c; msr_t msr;
- /* cpuid sets msr 0x8B iff a microcode update has been loaded. */ + /* cpuid sets msr 0x8B if a microcode update has been loaded. */
NACK. "IFF" is shorthand for "if and only if", see http://en.wikipedia.org/wiki/If_and_only_if
That's silly. This is not a mathematical expression nor a philosophical disquisition but a sentence. I am not even convinced that it was meant that way rather than being just a typo. If you have reasons to assume it means "If and only if" then let's write it that way. Stefan -- coresystems GmbH • Brahmsstr. 16 • D-79104 Freiburg i. Br. Tel.: +49 761 7668825 • Fax: +49 761 7664613 Email: info@coresystems.de • http://www.coresystems.de/ Registergericht: Amtsgericht Freiburg • HRB 7656 Geschäftsführer: Stefan Reinauer • Ust-IdNr.: DE245674866

On 20.02.2008 18:52, Stefan Reinauer wrote:
Carl-Daniel Hailfinger wrote:
On 20.02.2008 17:19, Stefan Reinauer wrote:
* ron minnich <rminnich@gmail.com> [071212 17:19]:
Question to you guys: why is the first wrmsr instruction there? From my understanding, by not properly initialising ECX, EAX and EDX this will overwrite whatever is in the MSR pointed to by ECX?!
BTW I tried out your code on our target hardware (Intel Celeron M, 600 MHz) and with that first wrmsr line in place it hangs and without it, it runs just fine.
Thanks Martin. That looks like quite a nice bug catch you've done :-)
Here's a patch that resolves the issue.
Signed-off-by: Stefan Reinauer <stepan@coresystems.de>
Index: src/cpu/intel/microcode/microcode.c =================================================================== --- src/cpu/intel/microcode/microcode.c (revision 3111) +++ src/cpu/intel/microcode/microcode.c (working copy) @@ -33,7 +33,6 @@ */ msr_t msr; __asm__ volatile ( - "wrmsr\n\t"
ACK.
"xorl %%eax, %%eax\n\t" "xorl %%edx, %%edx\n\t" "movl $0x8b, %%ecx\n\t" @@ -60,7 +59,7 @@ char *c; msr_t msr;
- /* cpuid sets msr 0x8B iff a microcode update has been loaded. */ + /* cpuid sets msr 0x8B if a microcode update has been loaded. */
NACK. "IFF" is shorthand for "if and only if", see http://en.wikipedia.org/wiki/If_and_only_if
That's silly. This is not a mathematical expression nor a philosophical disquisition but a sentence. I am not even convinced that it was meant that way rather than being just a typo. If you have reasons to assume it means "If and only if" then let's write it that way.
Merriam-Webster agrees with me that "iff" is a word: http://www.merriam-webster.com/dictionary/iff So this is a valid sentence and I see no reason to change it. Unless the bit can be set even of no microcode update has been uploaded, "iff" is the only correct word. Regards, Carl-Daniel -- http://www.hailfinger.org/

On Wed, Feb 20, 2008 at 1:09 PM, Carl-Daniel Hailfinger < c-d.hailfinger.devel.2006@gmx.net> wrote:
On 20.02.2008 18:52, Stefan Reinauer wrote:
Carl-Daniel Hailfinger wrote:
On 20.02.2008 17:19, Stefan Reinauer wrote:
* ron minnich <rminnich@gmail.com> [071212 17:19]:
Question to you guys: why is the first wrmsr instruction there? From my understanding, by not properly initialising ECX, EAX and EDX this will overwrite whatever is in the MSR pointed to by ECX?!
BTW I tried out your code on our target hardware (Intel Celeron M, 600 MHz) and with that first wrmsr line in place it hangs and without it, it runs just fine.
Thanks Martin. That looks like quite a nice bug catch you've done :-)
Here's a patch that resolves the issue.
Signed-off-by: Stefan Reinauer <stepan@coresystems.de>
Index: src/cpu/intel/microcode/microcode.c =================================================================== --- src/cpu/intel/microcode/microcode.c (revision 3111) +++ src/cpu/intel/microcode/microcode.c (working copy) @@ -33,7 +33,6 @@ */ msr_t msr; __asm__ volatile ( - "wrmsr\n\t"
ACK.
"xorl %%eax, %%eax\n\t" "xorl %%edx, %%edx\n\t" "movl $0x8b, %%ecx\n\t" @@ -60,7 +59,7 @@ char *c; msr_t msr;
- /* cpuid sets msr 0x8B iff a microcode update has been loaded. */ + /* cpuid sets msr 0x8B if a microcode update has been loaded. */
NACK. "IFF" is shorthand for "if and only if", see http://en.wikipedia.org/wiki/If_and_only_if
That's silly. This is not a mathematical expression nor a philosophical disquisition but a sentence. I am not even convinced that it was meant that way rather than being just a typo. If you have reasons to assume it means "If and only if" then let's write it that way.
Merriam-Webster agrees with me that "iff" is a word: http://www.merriam-webster.com/dictionary/iff So this is a valid sentence and I see no reason to change it. Unless the bit can be set even of no microcode update has been uploaded, "iff" is the only correct word.
Regards, Carl-Daniel
Meriam-Webster and wikipedia might know, but not everyone does. Why can't we just change it to "if and only if" instead of using an obscure word that looks like a typo? -Corey

On 20.02.2008 19:29, Corey Osgood wrote:
On Wed, Feb 20, 2008 at 1:09 PM, Carl-Daniel Hailfinger < c-d.hailfinger.devel.2006@gmx.net> wrote:
On 20.02.2008 18:52, Stefan Reinauer wrote:
Carl-Daniel Hailfinger wrote:
On 20.02.2008 17:19, Stefan Reinauer wrote:
* ron minnich <rminnich@gmail.com> [071212 17:19]:
> Question to you guys: why is the first wrmsr instruction there? > From my > understanding, by not properly initialising ECX, EAX and EDX this > will > overwrite whatever is in the MSR pointed to by ECX?! > > BTW I tried out your code on our target hardware (Intel Celeron M, > 600 MHz) > and with that first wrmsr line in place it hangs and without it, > it runs > just fine. > > Thanks Martin. That looks like quite a nice bug catch you've done :-)
Here's a patch that resolves the issue.
Signed-off-by: Stefan Reinauer <stepan@coresystems.de>
Index: src/cpu/intel/microcode/microcode.c =================================================================== --- src/cpu/intel/microcode/microcode.c (revision 3111) +++ src/cpu/intel/microcode/microcode.c (working copy) @@ -33,7 +33,6 @@ */ msr_t msr; __asm__ volatile ( - "wrmsr\n\t"
ACK.
"xorl %%eax, %%eax\n\t" "xorl %%edx, %%edx\n\t" "movl $0x8b, %%ecx\n\t" @@ -60,7 +59,7 @@ char *c; msr_t msr;
- /* cpuid sets msr 0x8B iff a microcode update has been loaded. */ + /* cpuid sets msr 0x8B if a microcode update has been loaded. */
NACK. "IFF" is shorthand for "if and only if", see http://en.wikipedia.org/wiki/If_and_only_if
That's silly. This is not a mathematical expression nor a philosophical disquisition but a sentence. I am not even convinced that it was meant that way rather than being just a typo. If you have reasons to assume it means "If and only if" then let's write it that
way.
Merriam-Webster agrees with me that "iff" is a word: http://www.merriam-webster.com/dictionary/iff So this is a valid sentence and I see no reason to change it. Unless the bit can be set even of no microcode update has been uploaded, "iff" is the only correct word.
Regards, Carl-Daniel
Meriam-Webster and wikipedia might know, but not everyone does. Why can't we just change it to "if and only if" instead of using an obscure word that looks like a typo?
OK, I was just used to the word from my Linux kernel and math background and thought everybody would understand it. "If and only if" is a longer phrase, but it seems to be understood by a wider audience, so I have dropped my objections against the change to that phrase. Regards, Carl-Daniel -- http://www.hailfinger.org/

Quoting Corey Osgood <corey.osgood@gmail.com>:
On Wed, Feb 20, 2008 at 1:09 PM, Carl-Daniel Hailfinger < c-d.hailfinger.devel.2006@gmx.net> wrote:
On 20.02.2008 18:52, Stefan Reinauer wrote:
Carl-Daniel Hailfinger wrote:
On 20.02.2008 17:19, Stefan Reinauer wrote:
* ron minnich <rminnich@gmail.com> [071212 17:19]:
> Question to you guys: why is the first wrmsr instruction there? > From my > understanding, by not properly initialising ECX, EAX and EDX this > will > overwrite whatever is in the MSR pointed to by ECX?! > > BTW I tried out your code on our target hardware (Intel Celeron M, > 600 MHz) > and with that first wrmsr line in place it hangs and without it, > it runs > just fine. > Thanks Martin. That looks like quite a nice bug catch you've done :-)
Here's a patch that resolves the issue.
Signed-off-by: Stefan Reinauer <stepan@coresystems.de>
Index: src/cpu/intel/microcode/microcode.c =================================================================== --- src/cpu/intel/microcode/microcode.c (revision 3111) +++ src/cpu/intel/microcode/microcode.c (working copy) @@ -33,7 +33,6 @@ */ msr_t msr; __asm__ volatile ( - "wrmsr\n\t"
ACK.
"xorl %%eax, %%eax\n\t" "xorl %%edx, %%edx\n\t" "movl $0x8b, %%ecx\n\t" @@ -60,7 +59,7 @@ char *c; msr_t msr;
- /* cpuid sets msr 0x8B iff a microcode update has been loaded. */ + /* cpuid sets msr 0x8B if a microcode update has been loaded. */
NACK. "IFF" is shorthand for "if and only if", see http://en.wikipedia.org/wiki/If_and_only_if
That's silly. This is not a mathematical expression nor a philosophical disquisition but a sentence. I am not even convinced that it was meant that way rather than being just a typo. If you have reasons to assume it means "If and only if" then let's write it that way.
Merriam-Webster agrees with me that "iff" is a word: http://www.merriam-webster.com/dictionary/iff So this is a valid sentence and I see no reason to change it. Unless the bit can be set even of no microcode update has been uploaded, "iff" is the only correct word.
Regards, Carl-Daniel
Meriam-Webster and wikipedia might know, but not everyone does. Why can't we just change it to "if and only if" instead of using an obscure word that looks like a typo?
-Corey
I agree, it's silly. Fist of all it is just a comment. Second, I doesn't effect the actual code. Looks like a typo to me.... Acked-by: Joseph Smith <joe@smittys.pointclark.net> Thanks - Joe
participants (5)
-
Carl-Daniel Hailfinger
-
Corey Osgood
-
joe@smittys.pointclark.net
-
ron minnich
-
Stefan Reinauer