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

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

[Eduardo Habkost]

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 <ehabkost@redhat.com>
---
Cc: Markus Armbruster <armbru@redhat.com>
Cc: brillywu@viatech.com.cn <brillywu@viatech.com.cn>
Cc: Marcelo Tosatti <mtosatti@redhat.com>
---
 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 {
-- 
1.7.11.7

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

[Markus Armbruster]

Eduardo Habkost <ehabkost@redhat.com> 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 <ehabkost@redhat.com>
> ---
> Cc: Markus Armbruster <armbru@redhat.com>
> Cc: brillywu@viatech.com.cn <brillywu@viatech.com.cn>
> Cc: Marcelo Tosatti <mtosatti@redhat.com>
> ---
>  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 <armbru@redhat.com>

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

[Anthony Liguori]

Eduardo Habkost <ehabkost@redhat.com> 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 <ehabkost@redhat.com>

Applied. Thanks.

Regards,

Anthony Liguori

> ---
> Cc: Markus Armbruster <armbru@redhat.com>
> Cc: brillywu@viatech.com.cn <brillywu@viatech.com.cn>
> Cc: Marcelo Tosatti <mtosatti@redhat.com>
> ---
>  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 {
> -- 
> 1.7.11.7