From mboxrd@z Thu Jan 1 00:00:00 1970 From: Radim =?utf-8?B?S3LEjW3DocWZ?= Subject: Re: [PATCH 4/6] KVM: x86: Fix determining flat mode in recalculate_apic_map Date: Wed, 1 Oct 2014 20:27:16 +0200 Message-ID: <20141001182715.GD12083@potion.brq.redhat.com> References: <1412099359-5316-1-git-send-email-namit@cs.technion.ac.il> <1412099359-5316-5-git-send-email-namit@cs.technion.ac.il> <20141001160442.GD12085@potion.brq.redhat.com> Mime-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: QUOTED-PRINTABLE Cc: Nadav Amit , pbonzini@redhat.com, kvm@vger.kernel.org To: Nadav Amit Return-path: Received: from mx1.redhat.com ([209.132.183.28]:46314 "EHLO mx1.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751780AbaJAS1X (ORCPT ); Wed, 1 Oct 2014 14:27:23 -0400 Content-Disposition: inline In-Reply-To: Sender: kvm-owner@vger.kernel.org List-ID: 2014-10-01 20:30+0300, Nadav Amit: > On Oct 1, 2014, at 7:04 PM, Radim Kr=C4=8Dm=C3=A1=C5=99 wrote: > > 2014-09-30 20:49+0300, Nadav Amit: > The condition =E2=80=9C!new->cid_mask=E2=80=9D is certainly always tr= ue (unless we already set the cid/lid according to cluster-mode setting= s). Exactly, it "optimizes" the switch to a non-default clustered mode. > I did not feel comfortable removing it without replacing it with some= thing =E2=80=9Cequivalent=E2=80=9D. >=20 > >=20 > >> Since we recalculate the apic map whenever the DFR is changed, the= bug appears > >> to have no effect, and perhaps the entire check can be removed. > >=20 > > It could, for the check is just an optimization. > > (This whole code deserves a rewrite though ...) >=20 > I did not hit such bug, but IMO the code is buggy. > The APIC mode is determined by processors that enabled their apic ena= bled (in the spurious vector), and all the enabled one should configure= the same mode. > Nonetheless, as stated in the SDM 10.4.7.2 "Local APIC State After It= Has Been Software Disabled APICs=E2=80=9D - the disabled APICs should = still respond to NMIs, INIT, etc. > So it appears the loop should be broken into two loops - first determ= ine the apic mode, according to the first enabled APIC. Then, iterate a= gain over vCPUs and build the logical map. Our assumption that all have the same mode is horrible. (Do they all have be the same?) The only thing we allow out of your scenario that I can see is software disabled x2apic after enabled clustered xapic processors and that doesn't need two loops, just a sw check at x2apic. Practically, it is a harmless bug :)