Message ID | 20170505142743.19849-2-berrange@redhat.com |
---|---|
State | New |
Headers | show |
On Fri, May 05, 2017 at 03:27:42PM +0100, Daniel P. Berrange wrote: > Change the nested if statements into a flat switch, to make > it clearer what validation / capping is being performed on > different CPUID index values. > > Signed-off-by: Daniel P. Berrange <berrange@redhat.com> > --- > target/i386/cpu.c | 43 +++++++++++++++++++++++++------------------ > 1 file changed, 25 insertions(+), 18 deletions(-) > > diff --git a/target/i386/cpu.c b/target/i386/cpu.c > index 13c0985..3d5903c 100644 > --- a/target/i386/cpu.c > +++ b/target/i386/cpu.c > @@ -2628,26 +2628,33 @@ void cpu_x86_cpuid(CPUX86State *env, uint32_t index, uint32_t count, > uint32_t pkg_offset; > > /* test if maximum index reached */ > - if (index & 0x80000000) { > + switch (index & 0xF0000000) { > + case 0: > + /* Intel documentation states that invalid EAX input will > + * return the same information as EAX=cpuid_level > + * (Intel SDM Vol. 2A - Instruction Set Reference - CPUID) > + */ > + if (index > env->cpuid_level) { > + index = env->cpuid_level; > + } > + break; It looks like we can just move this code to the default branch, and remove the "case 0:" branch. > + case 0x80000000: > if (index > env->cpuid_xlevel) { > - if (env->cpuid_xlevel2 > 0) { > - /* Handle the Centaur's CPUID instruction. */ > - if (index > env->cpuid_xlevel2) { > - index = env->cpuid_xlevel2; ^^^ [1] > - } else if (index < 0xC0000000) { > - index = env->cpuid_xlevel; ^^^ [2] > - } > - } else { > - /* 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; ^^^ [3] > - } > + index = env->cpuid_xlevel; ^^^ [4] Actually, CPUs do return the max _basic_ leaf (cpuid_level) even when input EAX is larger than 0x80000000. (See [3]) ...except for the existing code at [2], but: * TCG doesn't support any of the Centaur features. * KVM is not affected by [2] because CPUID limits are handled inside the KVM kernel code (and KVM returns CPUID[env->cpuid_level] on that case, see linux/arch/x86/kvm/cpuid.c:check_cpuid_limit()) In other words, [2] was dead code, and we only need to reproduce behavior implemented by [3]. > } > - } else { > - if (index > env->cpuid_level) > - index = env->cpuid_level; > + break; > + case 0xC0000000: > + if (index > env->cpuid_xlevel2) { > + index = env->cpuid_xlevel2; This line reimplements [1], but (like [2]) [1] was dead code too. But I suggest setting index=env->cpuid_level for consistency with KVM code. > + } > + break; > + default: > + /* 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; > + break; > } What about simplifying this even further: if (index > 0xC0000000) { limit = env->cpuid_xlevel2; } else if (index > 0x80000000) { limit = env->cpuid_xlevel; } else { limit = env->cpuid_level; } if (index > limit) { /* 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; } > > switch(index) { > -- > 2.9.3 >
diff --git a/target/i386/cpu.c b/target/i386/cpu.c index 13c0985..3d5903c 100644 --- a/target/i386/cpu.c +++ b/target/i386/cpu.c @@ -2628,26 +2628,33 @@ void cpu_x86_cpuid(CPUX86State *env, uint32_t index, uint32_t count, uint32_t pkg_offset; /* test if maximum index reached */ - if (index & 0x80000000) { + switch (index & 0xF0000000) { + case 0: + /* Intel documentation states that invalid EAX input will + * return the same information as EAX=cpuid_level + * (Intel SDM Vol. 2A - Instruction Set Reference - CPUID) + */ + if (index > env->cpuid_level) { + index = env->cpuid_level; + } + break; + case 0x80000000: if (index > env->cpuid_xlevel) { - if (env->cpuid_xlevel2 > 0) { - /* Handle the Centaur's CPUID instruction. */ - if (index > env->cpuid_xlevel2) { - index = env->cpuid_xlevel2; - } else if (index < 0xC0000000) { - index = env->cpuid_xlevel; - } - } else { - /* 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; - } + index = env->cpuid_xlevel; } - } else { - if (index > env->cpuid_level) - index = env->cpuid_level; + break; + case 0xC0000000: + if (index > env->cpuid_xlevel2) { + index = env->cpuid_xlevel2; + } + break; + default: + /* 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; + break; } switch(index) {
Change the nested if statements into a flat switch, to make it clearer what validation / capping is being performed on different CPUID index values. Signed-off-by: Daniel P. Berrange <berrange@redhat.com> --- target/i386/cpu.c | 43 +++++++++++++++++++++++++------------------ 1 file changed, 25 insertions(+), 18 deletions(-)