Re: [LinuxBIOS] Intel microcode revision code

[ron minnich]

On Dec 12, 2007 12:28 AM,   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

[LinuxBIOS] Intel microcode revision code

[Stefan Reinauer]

* ron minnich  [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.

[LinuxBIOS] Intel microcode revision code

[Carl-Daniel Hailfinger]

On 20.02.2008 17:19, Stefan Reinauer wrote:
> ron minnich  [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
> 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

[LinuxBIOS] Intel microcode revision code

[Stefan Reinauer]

Carl-Daniel Hailfinger wrote:
> On 20.02.2008 17:19, Stefan Reinauer wrote:
> > ron minnich  [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
> > 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

[LinuxBIOS] Intel microcode revision code

[Carl-Daniel Hailfinger]

On 20.02.2008 18:52, Stefan Reinauer wrote:
> Carl-Daniel Hailfinger wrote:
> > On 20.02.2008 17:19, Stefan Reinauer wrote:
> > > ron minnich  [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
> > > 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

[LinuxBIOS] Intel microcode revision code

[Corey Osgood]

On Wed, Feb 20, 2008 at 1:09 PM, Carl-Daniel Hailfinger <
[e-mail]> 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  [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
> > > > 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

[LinuxBIOS] Intel microcode revision code

[Carl-Daniel Hailfinger]

On 20.02.2008 19:29, Corey Osgood wrote:
> On Wed, Feb 20, 2008 at 1:09 PM, Carl-Daniel Hailfinger <
> [e-mail]> 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  [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
> > > > > 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

[LinuxBIOS] Intel microcode revision code

Quoting Corey Osgood :
> On Wed, Feb 20, 2008 at 1:09 PM, Carl-Daniel Hailfinger <
> [e-mail]> 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  [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
> > > > > 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

Thanks - Joe