Re: [Qemu-devel] [PATCH] target-i386: CPUID: return highest basic leaf if eax > cpuid_xlevel

[Markus Armbruster]

Eduardo Habkost <address@hidden> writes:

> This fixes a subtle bug. A bug that probably won't cause trouble for any
> existing OS, but a bug anyway:
>
> Intel SDM Volume 2, CPUID Instruction states:
>
>> Two types of information are returned: basic and extended function
>> information. If a value entered for CPUID.EAX is higher than the maximum
>> input value for basic or extended function for that processor then the
>> data for the highest basic information leaf is returned. For example,
>> using the Intel Core i7 processor, the following is true:
>>
>>   CPUID.EAX = 05H (* Returns MONITOR/MWAIT leaf. *)
>>   CPUID.EAX = 0AH (* Returns Architectural Performance Monitoring leaf. *)
>>   CPUID.EAX = 0BH (* Returns Extended Topology Enumeration leaf. *)
>>   CPUID.EAX = 0CH (* INVALID: Returns the same information as
>> CPUID.EAX = 0BH. *)
>>   CPUID.EAX = 80000008H (* Returns linear/physical address size data. *)
>>   CPUID.EAX = 8000000AH (* INVALID: Returns same information as
>> CPUID.EAX = 0BH. *)
>
> AMD's CPUID Specification, on the other hand, is less specific:
>
>> The CPUID instruction supports two sets or ranges of functions,
>> standard and extended.
>>
>> • The smallest function number of the standard function range is
>>   Fn0000_0000. The largest function num- ber of the standard function
>>   range, for a particular implementation, is returned in CPUID
>>   Fn0000_0000_EAX.
>>
>> • The smallest function number of the extended function range is
>>   Fn8000_0000. The largest function num- ber of the extended function
>>   range, for a particular implementation, is returned in CPUID
>>   Fn8000_0000_EAX.
>>
>> Functions that are neither standard nor extended are undefined and
>> should not be relied upon.
>
> QEMU's behavior matched Intel's specification before, but this was
> changed by commit b3baa152aaef1905876670590275c2dd0bbb088c. This patch
> restores the behavior documented by Intel when cpuid_xlevel2 is 0.
>
> The existing behavior when cpuid_xlevel2 is set (falling back to
> level=cpuid_xlevel) is being kept, as I couldn't find any public
> documentation on the CPUID 0xC0000000 function range on Centaur CPUs.
>
> Signed-off-by: Eduardo Habkost <address@hidden>
> ---
> Cc: Markus Armbruster <address@hidden>
> Cc: address@hidden <address@hidden>
> Cc: Marcelo Tosatti <address@hidden>
> ---
>  target-i386/cpu.c | 6 +++++-
>  1 file changed, 5 insertions(+), 1 deletion(-)
>
> diff --git a/target-i386/cpu.c b/target-i386/cpu.c
> index 1837f5a..3cd1cee 100644
> --- a/target-i386/cpu.c
> +++ b/target-i386/cpu.c
> @@ -1648,7 +1648,11 @@ void cpu_x86_cpuid(CPUX86State *env, uint32_t index, 
> uint32_t count,
>                      index = env->cpuid_xlevel;
>                  }
>              } else {
> -                index =  env->cpuid_xlevel;
> +                /* Intel documentation states that invalid EAX input will
> +                 * return the same information as EAX=cpuid_level
> +                 * (Intel SDM Vol. 2A - Instruction Set Reference - CPUID)
> +                 */
> +                index =  env->cpuid_level;
>              }
>          }
>      } else {

Trivia: brought to my attention by when Coverity voiced suspicions on
the old code we still have in RHEL-6:

    if (index & 0x80000000) {
        if (index > env->cpuid_xlevel)
            index = env->cpuid_level;
    } else {
        if (index > env->cpuid_level)
            index = env->cpuid_level;
    }

Looks just like a pasto, doesn't it?  But it's correct!  Commit b3baa152
silently "fixed" the pasto.  This patch reverts that mistake.  Thanks
Eduardo for adding the explanatory comment.

Reviewed-by: Markus Armbruster <address@hidden>