From mboxrd@z Thu Jan 1 00:00:00 1970 From: Avi Kivity Subject: Re: [PATCH 2/2] kvm/x86: remove unneeded substitute search for missing CPUID entries Date: Thu, 31 Mar 2011 12:32:47 +0200 Message-ID: <4D94584F.1020409@redhat.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> <4D945375.4060407@amd.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: Andre Przywara Return-path: Received: from mx1.redhat.com ([209.132.183.28]:25234 "EHLO mx1.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752133Ab1CaKcv (ORCPT ); Thu, 31 Mar 2011 06:32:51 -0400 In-Reply-To: <4D945375.4060407@amd.com> Sender: kvm-owner@vger.kernel.org List-ID: 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