Re: [PATCH 4/6] KVM: x86: Fix determining flat mode in recalculate_apic_map

["Radim Krčmář" <rkrcmar@redhat.com>]

2014-10-01 20:30+0300, Nadav Amit:
> On Oct 1, 2014, at 7:04 PM, Radim Krčmář <rkrcmar@redhat.com> wrote:
> > 2014-09-30 20:49+0300, Nadav Amit:
> The condition “!new->cid_mask” is certainly always true (unless we already set the cid/lid according to cluster-mode settings).

Exactly, it "optimizes" the switch to a non-default clustered mode.

> I did not feel comfortable removing it without replacing it with something “equivalent”.
> 
> > 
> >> 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.
> > 
> > It could, for the check is just an optimization.
> > (This whole code deserves a rewrite though ...)
> 
> I did not hit such bug, but IMO the code is buggy.
> The APIC mode is determined by processors that enabled their apic enabled (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” - the disabled APICs should still respond to NMIs, INIT, etc.
> So it appears the loop should be broken into two loops - first determine the apic mode, according to the first enabled APIC. Then, iterate again 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 :)