From mboxrd@z Thu Jan 1 00:00:00 1970 From: Andre Przywara Subject: Re: [PATCH 2/2] kvm/x86: remove unneeded substitute search for missing CPUID entries Date: Thu, 31 Mar 2011 12:12:05 +0200 Message-ID: <4D945375.4060407@amd.com> References: <1301490106-20626-1-git-send-email-andre.przywara@amd.com> <1301490106-20626-2-git-send-email-andre.przywara@amd.com> <4D932F69.8070205@redhat.com> Mime-Version: 1.0 Content-Type: text/plain; charset="ISO-8859-1"; format=flowed Content-Transfer-Encoding: 7bit Cc: "kvm@vger.kernel.org" To: Avi Kivity Return-path: Received: from tx2ehsobe001.messaging.microsoft.com ([65.55.88.11]:57845 "EHLO TX2EHSOBE001.bigfish.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751614Ab1CaKVe (ORCPT ); Thu, 31 Mar 2011 06:21:34 -0400 In-Reply-To: <4D932F69.8070205@redhat.com> Sender: kvm-owner@vger.kernel.org List-ID: 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. 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. Regards, Andre. -- Andre Przywara AMD-Operating System Research Center (OSRC), Dresden, Germany