Re: [PATCH 14/21] KVM: x86: Software disabled APIC should still deliver NMIs

[Nadav Amit <nadav.amit@gmail.com>]

Paolo Bonzini <pbonzini@redhat.com> wrote:

> 
> 
> On 06/11/2014 17:45, Radim Krčmář wrote:
>> 2014-11-06 10:34+0100, Paolo Bonzini:
>>> On 05/11/2014 21:45, Nadav Amit wrote:
>>>> If I understand the SDM correctly, in such scenario (all APICs are
>>>> software disabled) the mode is left as the default - flat mode (see
>> 
>> APIC doesn't have any global mode (it is just KVM's simplification), so
>> when a message lands on the system bus, it just compares MDA with LDR
>> and DFR ...
>> 
>>>> section 10.6.2.2 "Logical Destination Mode”): "All processors that
>>>> have their APIC software enabled (using the spurious vector
>>>> enable/disable bit) must have their DFRs (Destination Format
>>>> Registers) programmed identically. The default mode for DFR is flat
>>>> mode.”
>> 
>> I think the "default mode" points to reset state, which is flat DFR;
>> and it is mentioned only because of the following sentence
>>  If you are using cluster mode, DFRs must be programmed before the APIC
>>  is software enabled.
>> 
>>> That's not what either Bochs or QEMU do, though.  (Though in the case of
>>> Bochs I cannot find the place where reception of IPIs is prevented for
>>> software-disabled APICs, so I'm not sure how much to trust it in this case).
>>> 
>>> I'm not sure why software-disabled APICs could have different DFRs, if
>>> the APICs can receive NMI IPIs.  I'll ask Intel.
>> 
>> When changing the mode, we can't switch DFR synchronously, so it has to
>> happen and NMI may be needed (watchdog?) before APIC configuration.
>> Explicit statement might have been a hint to be _extra_ careful when
>> using logical destination for INIT, NMI, ... I wonder what they'll say.
>> 
>> Anyway, Paolo's patch seems to be in the right direction, I'd modify it
>> a bit though:
>> 
>> LDR=0 isn't addressable in any logical mode, so we can ignore APICs that
>> don't set it and decide the mode by the last nonzero one.
>> This works in a situation, where one part is configured for cluster and
>> the rest is still in reset state.
>> 
>> (It gets harder if we allow nonzero LDRs with different DFR ...
>> we'd need to split our logical map to handle it.)
>> 
>> diff --git a/arch/x86/kvm/lapic.c b/arch/x86/kvm/lapic.c
>> index 758f838..6da303e1 100644
>> --- a/arch/x86/kvm/lapic.c
>> +++ b/arch/x86/kvm/lapic.c
>> @@ -148,10 +148,6 @@ static void recalculate_apic_map(struct kvm *kvm)
>> 		goto out;
>> 
>> 	new->ldr_bits = 8;
>> -	/* flat mode is default */
>> -	new->cid_shift = 8;
>> -	new->cid_mask = 0;
>> -	new->lid_mask = 0xff;
>> 	new->broadcast = APIC_BROADCAST;
>> 
>> 	kvm_for_each_vcpu(i, vcpu, kvm) {
>> @@ -166,7 +162,7 @@ static void recalculate_apic_map(struct kvm *kvm)
>> 			new->cid_mask = (1 << KVM_X2APIC_CID_BITS) - 1;
>> 			new->lid_mask = 0xffff;
>> 			new->broadcast = X2APIC_BROADCAST;
>> -		} else if (kvm_apic_hw_enabled(apic)) {
>> +		} else if (kvm_apic_get_reg(apic, APIC_LDR)) {
>> 			if (kvm_apic_get_reg(apic, APIC_DFR) ==
>> 							APIC_DFR_CLUSTER) {
>> 				new->cid_shift = 4;
> 
> I merged this patch and Nadav’s.

Sorry for the late and long reply, but I got an issue with the new version
(and my previous version as well). Indeed, the SDM states that DFR should
be the same for enabled CPUs, and that the BIOS should get all CPUs in
either xAPIC or x2APIC. Yet, there is nothing that says all CPUs need to be
in xAPIC/x2APIC mode.

In my tests (which pass on bare-metal), I got a scenario in which some CPUs
are in xAPIC mode, the BSP changed (which is currently not handled correctly
by KVM) and the BSP has x2APIC enabled. All the core APICs are
software-enabled. The expected behaviour is that the CPUs with x2APIC
enabled would work in x2APIC mode.

I think such a transitory scenario is possible on real-systems as well,
perhaps during CPU hot-plug. It appears the previous version (before all of
our changes) handled it better. I presume the most efficient way is to start
determining the APIC logical mode from the BSP, and if it is disabled,
traverse the rest of the CPUs until finding the first one with APIC enabled.
Yet, I have not finished doing and checking the BSP fix and other dependent
INIT signal handling fixes.

In the meanwhile, would you be ok with restoring some of the previous
behaviour - i.e., x2APIC is enabled if any CPU turned it on (regardless to
whether APIC is software enabled), otherwise use the configuration of the
last enabled APIC?

-- >8 —
Subject: [PATCH] KVM: x86: Traverse all CPUs during recalculate_apic_map

---
 arch/x86/kvm/lapic.c | 20 +++++++++++---------
 1 file changed, 11 insertions(+), 9 deletions(-)

diff --git a/arch/x86/kvm/lapic.c b/arch/x86/kvm/lapic.c
index 9c90d31..6dc2be6 100644
--- a/arch/x86/kvm/lapic.c
+++ b/arch/x86/kvm/lapic.c
@@ -139,6 +139,7 @@ static void recalculate_apic_map(struct kvm *kvm)
 	struct kvm_apic_map *new, *old = NULL;
 	struct kvm_vcpu *vcpu;
 	int i;
+	bool any_enabled = false;
 
 	new = kzalloc(sizeof(struct kvm_apic_map), GFP_KERNEL);
 
@@ -160,13 +161,21 @@ static void recalculate_apic_map(struct kvm *kvm)
 		if (!kvm_apic_present(vcpu))
 			continue;
 
+		/*
+		 * All APICs DFRs have to be configured the same mode by an OS.
+		 * We take advatage of this while building logical id lookup
+		 * table. After reset APICs are in software disabled mode, so if
+		 * we find apic with different setting we assume this is the mode
+		 * OS wants all apics to be in; build lookup table accordingly.
+		 */
 		if (apic_x2apic_mode(apic)) {
 			new->ldr_bits = 32;
 			new->cid_shift = 16;
 			new->cid_mask = (1 << KVM_X2APIC_CID_BITS) - 1;
 			new->lid_mask = 0xffff;
 			new->broadcast = X2APIC_BROADCAST;
-		} else if (kvm_apic_get_reg(apic, APIC_LDR)) {
+			break;
+		} else if (!any_enabled && kvm_apic_get_reg(apic, APIC_LDR)) {
 			if (kvm_apic_get_reg(apic, APIC_DFR) ==
 							APIC_DFR_CLUSTER) {
 				new->cid_shift = 4;
@@ -179,15 +188,8 @@ static void recalculate_apic_map(struct kvm *kvm)
 			}
 		}
 
-		/*
-		 * All APICs have to be configured in the same mode by an OS.
-		 * We take advatage of this while building logical id loockup
-		 * table. After reset APICs are in software disabled mode, so if
-		 * we find apic with different setting we assume this is the mode
-		 * OS wants all apics to be in; build lookup table accordingly.
-		 */
 		if (kvm_apic_sw_enabled(apic))
-			break;
+			any_enabled = true;
 	}
 
 	kvm_for_each_vcpu(i, vcpu, kvm) {
-- 
1.9.1