Re: [PATCH 2/2] kvm/x86: remove unneeded substitute search for missing CPUID entries

[Avi Kivity <avi@redhat.com>]

On 03/31/2011 12:12 PM, Andre Przywara wrote:
> Avi Kivity wrote:
>> On 03/30/2011 03:01 PM, Andre Przywara wrote:
>>> If KVM cannot find an exact match for a requested CPUID leaf, the
>>> code will try to find the closest match instead of simply confessing
>>> it's failure. The heuristic is on one hand wrong nowadays,
>>> since it does not take the KVM CPUID leaves (0x400000xx) into
>>> account. On the other hand the callers of this function can all deal
>>> with the no-match situation. So lets remove this code, as it serves
>>> no purpose.
>>> This fixes a crash of newer Linux kernels as KVM guests on
>>> AMD Bulldozer CPUs, where bogus values were returned in response to
>>> a CPUID intercept.
>>>
>>>
>>> @@ -4959,12 +4959,6 @@ struct kvm_cpuid_entry2 
>>> *kvm_find_cpuid_entry(struct kvm_vcpu *vcpu,
>>>               best = e;
>>>               break;
>>>           }
>>> -        /*
>>> -         * Both basic or both extended?
>>> -         */
>>> -        if (((e->function ^ function)&  0x80000000) == 0)
>>> -            if (!best || e->function>  best->function)
>>> -                best = e;
>>>       }
>>>       return best;
>>>   }
>>
>>
>> This behaviour is mandated by the spec (looking at the Intel one), 
>> though it is implemented incorrectly - should always return largest 
>> basic leaf, and ignore the kvm leaves.
>
> But the spec says that this applies only if EAX is higher than the 
> largest supported leaf. The code as is checks whether KVM has an entry 
> in the cpuid "cache" for it, which is not the same. Especially this 
> case that hit me was a missing index entry, which should return 0.

Ah, I see.

> The check for too large leaf numbers should be moved into 
> kvm_emulate_cpuid(). There is already some code in QEMU 
> (cpu_x86_cpuid) to handle this, but that path does not apply to KVM.
>
> I will make a new version of this patch which replaces the old check 
> with a sane version in kvm_emulate_cpuid().
>
> Thanks for pointing this out.
>
>>
>> I think the correct behaviour is:
>>
>>     if (e->function < 10000 && (!best || e->function > best->function))
>>          best = e;
>>
>> We probably need a find_exact_cpuid_entry() that returns NULL if it 
>> doesn't find a match, for internal use.
>
> As mentioned, this behavior only applies to the actual intercept case, 
> not to all users of kvm_find_cpuid_entry(). So I'd like to make this 
> check in the intercept code path and not in this function.
>

Right.

-- 
error compiling committee.c: too many arguments to function