Questionable code in cpu_type() in versions 6.x and 4.x.

[Chip Programmer]

One of the first condition checks in the function that tries get the processor identification string, there is a bug in the code. The condition check looks for a 0x0F in the first byte of the CPU Vendor string where one would expect to find "GenuineIntel". What the function wants to do is to identify Intel Pentium 4 type processors, but the condition check should be inside the Intel section, and should be looking at the CPUID value rather than the first byte of the Vendor string.

Furthermore, the same function that picks the "586" string is lacking "break" instructions in the switch statement. This means that although the function might think it is choosing "586", or "686", the actual string chosen is "Unidentified Processor".

[David (PassMark)]

We are not aware of any instances of mis-identification of real CPUs.

Do you have a specific example of a CPU model that returns the wrong identification.

[Chip Programmer]

Here is cpy_type() from the 4.x source:

/*
* Find CPU type
*/
void cpu_type(void)
{

<snipped>
/* Unknown processor */
default:

/* Make a guess at the family */
switch(cpu_id.vers.bits.family) {
case 5:

cprint(0, COL_MID, "586");

case 6:

cprint(0, COL_MID, "686");

default:

cprint(0, COL_MID, "Unidentified Processor");

}

}

}

From the above code snippet, it is quite obvious that "break" statements are missing after the cprint() function calls. If the CPUID value is from family 5, then the first case statement will fall through to the end.

For the other issue I noted, I'll have to show you the disassembled code to prove my point. However, do us all a favor and pass this thread to the developer(s) so the code can be repaired.

Even the 4.x version of cpu_type() has a problem dealing with Intel versus Transmeta processors.

A simple review of the source would tell us/you that.

[keith]

Thanks for pointing the bugs out.

As v4.x is open source, we are open to accepting source code patches and incorporating into our code base. So feel free to submit any fixes to any bugs you may have found.

As David mentioned, it would be good to see a specific example which shows the wrong identification, such as a screenshot of the trace (in v4) or the MemTest86.log file (in v6).

[Chip Programmer]

Here is some C code that represents what I found in the 6.2.0 EFI binary.

void cpu_type(void)
{
    DWORD myEAX;
    DWORD myECX;
    DWORD myEDX;
    char *pProcessorString;

    pProcessorString = NULL;
    myEAX = szCPU_VendorId[0];
    myEDX = 0x0F;

    // This first condition is not possible.  It should be located lower, where Intel CPUs are checked.
    switch( myEAX )
    {
    case 0x0F:                                      // szCPU_VendorId[0] == 0x0F?
        myEAX = dCpuidValue;
        myEAX >>= 4;
        myEAX &= myEDX;
        switch( myEAX )
        {
        case 0:
        case 1:
        case 2:
            if (dCacheSize == 128)
                pProcessorString = "Celeron";
            else
                pProcessorString = "Pentium 4";
            break;

        case 3:
        case 4:
            if (dCacheSize == 256)
                pProcessorString = "Celeron (0.09)";
            else
                pProcessorString = "Pentium 4 (0.09)";
            break;
        
        case 6:
            pProcessorString = "Pentium D (65nm)";
            break;
        
        default:
            pProcessorString = "Unknown Intel";
            break;
        }
        break;
    
    case 'A':   // szCPU_VendorId[0] == 'A'?
        myEAX = dCpuidValue;
        myECX = myEAX;
        myECX >>= 8;
        myECX &= myEDX;
        switch( myECX )
        {
        case 4:
            myEAX >>= 4;
            myEAX &= myEDX;
            switch( myEAX )
            {
            case 3:
                pProcessorString = "AMD 486DX2";
                break;

            case 7:
                pProcessorString = "AMD 486DX2-WB";
                break;

            case 8:
                pProcessorString = "AMD 486DX4";
                break;

            case 9:
                pProcessorString = "AMD 486DX4-WB";
                break;

            case 14:
                pProcessorString = "AMD 5x86-WT";
                break;

            case 15:
                pProcessorString = "AMD 5x86-WB";
                break;
            }
            break;

        case 5:
            myEAX >>= 4;
            myEAX &= myEDX;
            switch( myEAX )
            {
            case 0:
            case 1:
            case 2:
            case 3:
                pProcessorString = "AMD K5";
                break;

            case 6:
            case 7:
                pProcessorString = "AMD K6";
                break;

            case 8:
                pProcessorString = "AMD K6-2";
                break;

            case 9:
                pProcessorString = "AMD K6-III";
                break;

            case 13:
                pProcessorString = "AMD K6-III+";
                break;
            }
            break;

        case 6:
            myEAX >>= 4;
            myEAX &= myEDX;
            switch( myEAX )
            {
            case 1:
                pProcessorString = "AMD Athlon (0.25)";
                break;

            case 2:
            case 4:
                pProcessorString = "AMD Athlon (0.18)";
                break;

            case 6:
                if (dCacheSize == 64)
                    pProcessorString = "AMD Duron (0.18)";
                else
                    pProcessorString = "Athlon XP (0.18)";
                break;

            case 8:
            case 10:
                if (dCacheSize == 64)
                    pProcessorString = "AMD Duron (0.13)";
                else
                    pProcessorString = "Athlon XP (0.13)";
                break;

            case 3:
            case 7:
                pProcessorString = "AMD Duron";
                break;
            }
            break;
        }
        break;

    case 'C':   // szCPU_VendorId[0] == 'C'?
        if (szCPU_VendorId[1] == 'e')
        {
            myECX = dCpuidValue;
            myEAX = myECX;
            myEAX >>= 8;
            myEAX &= myEDX;
            if (myEAX == 5)
                pProcessorString = "Centaur 5x86";
            else
            if (myEAX == 6)
            {
                myEAX = myECX;
                myEAX >>= 4;
                myEAX &= myEDX;
                if (myEAX == 0x0A)
                    pProcessorString = "VIA C7 (C5J)";
                else
                if (myEAX == 0x0D)
                    pProcessorString = "VIA C7 (C5R)";
                else
                if (myEAX == 0x0F)
                    pProcessorString = "VIA Isaiah (CN)";
                else
                if ((myECX & 0x0F) < 8)
                    pProcessorString = "VIA C3 Samuel2";
                else
                    pProcessorString = "VIA C3 Eden";
            }
        }
        else
        {
            myEAX = dCpuidValue;
            myECX = myEAX;
            myECX >>= 8;
            myECX &= myEDX;
            if (myECX == 0x05)
            {
                myEAX >>= 4;
                myEAX &= myEDX;
                if (myEAX == 0)
                    pProcessorString = "Cyrix 6x86MX/MII";
                else
                if (myEAX == 4)
                    pProcessorString = "Cyrix GXm";
            }
            else
            if (myECX == 6)
            {
                myECX = myEAX;
                myECX >>= 4;
                myECX &= myEDX;
                switch( myECX )
                {
                case 6:
                    pProcessorString = "Cyrix III";
                    break;

                case 7:
                    if ((myEAX & 0x0F) < 8)
                        pProcessorString = "VIA C3 Samuel2";
                    else
                        pProcessorString = "VIA C3 Ezra-T";
                    break;

                case 8:
                    pProcessorString = "VIA C3 Ezra-T";
                    break;

                case 9:
                    pProcessorString = "VIA C3 Nehemiah";
                    break;
                }
            }
        }
        break;

    case 'G':
        myEAX = dCpuidValue;
        if (szCPU_VendorId[7] == 'T')   // "GeniuneTMx86" ?
        {
            myEAX >>= 8;                // Get CPUID family value
            myEAX &= myEDX;
            if (myEAX == 5)             // CPUID == 0x000005xx?
                pProcessorString = "TM 5x00";
            else
            if (myEAX == 15)
                pProcessorString = "TM 8x00";
        }
        else
        {
            myECX = myEAX;              // Only other supported CPU whose VendorID string starts with 'G' is GenuineIntel.
            myECX >>= 8;                // Get CPUID family value
            myECX &= myEDX;
            if (myECX == 4)             // 80486 processor?
            {
                myEAX >>= 4;            // Is 80486.  Get the model value.
                myEAX &= myEDX;
                switch( myEAX )
                {
                case 0:
                case 1:
                    pProcessorString = "Intel 486DX";
                    break;

                case 2:
                    pProcessorString = "Intel 486SX";
                    break;

                case 3:
                    pProcessorString = "Intel 486DX2";
                    break;

                case 4:
                    pProcessorString = "Intel 486SL";
                    break;

                case 5:
                    pProcessorString = "Intel 486SX2";
                    break;

                case 6:
                    goto cpu_type_done;

                case 7:
                    pProcessorString = "Intel 486DX2-WB";
                    break;

                case 8:
                    pProcessorString = "Intel 486DX4";
                    break;

                case 9:
                    pProcessorString = "Intel 486DX4-WB";
                    break;
                }
            }
            else
            {
                myECX &= 0xFFFFFFFB;    // Turn off bit 2 of the CPUID family value
                switch( myECX )
                {
                case 5:
                    myEAX >>= 4;
                    myEAX &= myEDX;
                    if (myEAX <= 3)
                        pProcessorString = "Pentium";
                    else
                    if (myEAX == 4)
                        pProcessorString = "Pentium-MMX";
                    else
                    if (myEAX == 7)
                        pProcessorString = "Pentium";
                    else
                    if (myEAX == 8)
                        pProcessorString = "Pentium-MMX";

                    break;

                case 6:
                    myEAX >>= 4;
                    myEAX &= myEDX;
                    switch( myEAX )
                    {
                    case 0:
                    case 1:
                        pProcessorString = "Pentium Pro";
                        break;

                    case 3:
                    case 4:
                        pProcessorString = "Pentium II";
                        goto cpu_type_done;

                    case 5:
                        if (dCacheSize == 0)
                            pProcessorString = "Celeron";
                        else
                            pProcessorString = "Pentium II";
                        break;

                    case 6:
                        if (dCacheSize == 128)
                            pProcessorString = "Celeron";
                        else
                            pProcessorString = "Pentium II";
                        break;

                    default:
                        break;
                    }
                    break;

                case 7:
                case 8:
                case 11:
                    if (dCacheSize == 128)
                        pProcessorString = "Celeron";
                    else
                        pProcessorString = "Pentium III";
                    break;

                case 9:
                    if (dCacheSize == 512)
                        pProcessorString = "Celeron M (0.13)";
                    else
                        pProcessorString = "Pentium M (0.13)";
                    break;

                case 10:
                    pProcessorString = "Pentium III Xeon";
                    break;

                case 12:
                    pProcessorString = "Atom (0.045)";
                    break;

                case 13:
                    if (dCacheSize == 1024)
                        pProcessorString = "Celeron M (0.09)";
                    else
                        pProcessorString = "Pentium M (0.09)";
                    break;

                case 14:
                    pProcessorString = "Intel Core";
                    break;

                case 15:
                    if (dCacheSize == 1024)
                        pProcessorString = "Pentium E";
                    else
                        pProcessorString = "Intel Core 2";
                    break;
                }
            }
        }
        break;

    default:
        myEAX = dCpuidValue;
        myEAX >>= 8;           ; // Get CPUID family value
        myEAX &= myEDX;
        switch( myEAX )
        {
        case 5:
            pProcessorString = "586";
            //break; // This is missing.

        case 6:
            pProcessorString = "686";
            //break; // This is missing.

        default:
            pProcessorString = "Unidentified Processor";
            break;
        }
        break;
    }

cpu_type_done:
    return;
}

[Chip Programmer]

Now here is your 6.2.0 EFI binary after disassembly, with some comments.

; =============== S U B R O U T I N E =======================================

; Attributes: bp-based frame

cpu_type        proc near

var_C0          = byte ptr -0C0h
var_40          = byte ptr -40h

                push    ebp
                mov     ebp, esp
                sub     esp, 0C0h
                cmp     ds:dMaxExtendedCpuidValue, 80000004h
                jb      short loc_7EE07
                mov     eax, offset szCPU_BrandId
                lea     ecx, [ebp+var_40]
                call    sub_8266D
                lea     eax, [ebp+var_40]
                push    eax
                call    sub_7ED04
                pop     ecx
                lea     eax, [ebp+var_40]
                push    eax
                push    offset aA_0     ; "%a"
                lea     eax, [ebp+var_C0]
                push    80h ; '€'
                push    eax
                call    sub_8293A
                add     esp, 10h
                lea     eax, [ebp+var_C0]
                jmp     loc_7F2E2
; ---------------------------------------------------------------------------

loc_7EE07:
                movsx   eax, ds:szCPU_VendorId
                push    0Fh
                pop     edx             ; EDX = 0x0F, for saving nibble data
                sub     eax, edx
                jz      checkIntelPentium4 ; case (szCPU_VendorId[0] == 0x0F)
                sub     eax, 32h
                jz      checkAMD_Processors ; case (szCPU_VendorId[0] == 'A')
                dec     eax
                dec     eax
                jz      checkViaCentaurProcessor ; case (szCPU_VendorId[0] == 'C')
                sub     eax, 4
                mov     eax, ds:dCpuidValue
                jz      short checkIfIntelOrTransmeta ; case (szCPU_VendorId[0] == 'G')
                shr     eax, 8          ; default (szCPU_VendorId[0] == unknown vendor id string)
                                        ; Get CPUID Family value into AL
                and     eax, edx        ; Keep only the Family value
                sub     eax, 5
                jz      short cpu_type_choose_586 ; case (Family value == 5)
                dec     eax
                jz      short cpu_type_choose_686 ; case (Family value == 6)
                jmp     short cpu_type_choose_UnidentifiedProcessor ; case (Family value == unknown)
; ---------------------------------------------------------------------------

cpu_type_choose_586:
                mov     eax, offset a586 ; "586"
                mov     ecx, esi
                call    cprint

cpu_type_choose_686:
                mov     eax, offset a686 ; "686"
                mov     ecx, esi
                call    cprint

cpu_type_choose_UnidentifiedProcessor:
                mov     eax, offset aUnidentifiedPr ; "Unidentified Processor"
                jmp     loc_7F2E2
; ---------------------------------------------------------------------------

checkIfIntelOrTransmeta:

...

<snipped>

At the label loc_7EE07, the assembly code loads EDX register with 0x0F for nibble manipulation of the CPUID value. At the same time, EAX register is loaded with the first character of the CPU Vendor ID string, sign extended from AL into EAX.

The first instruction after these two registers are loaded is to check if the first character of the CPU Vendor ID string is equal to 0x0F, and perform some actions if this is true. These actions are intended to identify various Intel Pentium IV processors.

This comparison in assembly code is the first "case" statement after a "switch" statement. The second major comparison in assembly code is to look for the letter 'A' in the first character of the CPU Vendor ID string, believing this to be for AuthenticAMD and to perform AMD processor identification. The first comparison is for the letter 'C', which represents "CentaurHauls" and "CyrixInstead" Vendor ID strings. The assembly code continues, but you should get the picture that the "case 15:" is misplaced.

[keith]

Thanks for the disassembly analysis. We were able to locate the bug in the code.

It looks like there was a misplaced bracket in the 'Intel' case, in the following v4 code.

...
case 6:
            switch(cpu_id.vers.bits.model) {
            case 0:
            case 1:
                cprint(0, COL_MID, "Pentium Pro");
                break;
            case 3:
            case 4:
                cprint(0, COL_MID, "Pentium II");
                break;
            case 5:
                if (l2_cache == 0) {
                    cprint(0, COL_MID, "Celeron");
                } else {
                    cprint(0, COL_MID, "Pentium II");
                }
                break;
            case 6:
                  if (l2_cache == 128) {
                    cprint(0, COL_MID, "Celeron");
                  } else {
                    cprint(0, COL_MID, "Pentium II");
                  }
                } // This should not be here
                break;
...

This would most likely affect users of v4 (BIOS) more than v6 (UEFI) due to the fact that systems with UEFI would likely be using a newer CPU with a brand ID string, without the need to go through the switch/case mess. Nevertheless, this has been fixed for the next release.

Thanks for your time and effort.