| Message ID | 1356029028-7356-1-git-send-email-ehabkost@redhat.com |
|---|---|
| State | New |
| Headers | show |
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>
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
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 {