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

From: Nadav Amit <nadav.amit@gmail.com>
To: Paolo Bonzini <pbonzini@redhat.com>
Cc: Nadav Amit <namit@cs.technion.ac.il>, kvm@vger.kernel.org
Subject: Re: [PATCH 14/21] KVM: x86: Software disabled APIC should still deliver NMIs
Date: Wed, 5 Nov 2014 22:45:49 +0200	[thread overview]
Message-ID: <5D182FD6-0365-4471-AD28-C587A624D151@gmail.com> (raw)
In-Reply-To: <545A1852.1000603@redhat.com>

> On Nov 5, 2014, at 14:30, Paolo Bonzini <pbonzini@redhat.com> wrote:
> 
> 
> 
> On 02/11/2014 10:54, Nadav Amit wrote:
>> Currently, the APIC logical map does not consider VCPUs whose local-apic is
>> software-disabled.  However, NMIs, INIT, etc. should still be delivered to such
>> VCPUs. Therefore, the APIC mode should first be determined, and then the map,
>> considering all VCPUs should be constructed.
>> 
>> To address this issue, first find the APIC mode, and only then construct the
>> logical map.

[snip]

> 
> We need to take into account DFR even if all APICs are software disabled,
> in case it will be used to send NMIs.
> 
> What about this on top of your patch?
> 
> diff --git a/arch/x86/kvm/lapic.c b/arch/x86/kvm/lapic.c
> index c98b44d0ffe6..d8b11f47f45e 100644
> --- a/arch/x86/kvm/lapic.c
> +++ b/arch/x86/kvm/lapic.c
> @@ -160,29 +160,34 @@ static void recalculate_apic_map(struct kvm *kvm)
> 		if (!kvm_apic_present(vcpu))
> 			continue;
> 
> -		/*
> -		 * 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 xapic/flat 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;
> -			break;
> -		} else if (kvm_apic_sw_enabled(apic)) {
> +		} else if (kvm_apic_hw_enabled(apic)) {
> 			if (kvm_apic_get_reg(apic, APIC_DFR) ==
> 							APIC_DFR_CLUSTER) {
> 				new->cid_shift = 4;
> 				new->cid_mask = 0xf;
> 				new->lid_mask = 0xf;
> +			} else {
> +				new->cid_shift = 8;
> +				new->cid_mask = 0;
> +				new->lid_mask = 0xff;
> 			}
> -			break;
> 		}
> +
> +		/*
> +		 * 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;
> 	}
> 
> 	kvm_for_each_vcpu(i, vcpu, kvm) {
> 

I didn’t encounter the problem you try to fix, so I can’t say for sure, yet…

If I understand the SDM correctly, in such scenario (all APICs are software disabled) the mode is left as the default - flat mode (see 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.”

So I think the previous behaviour (before the additional changes) is the correct one.
I might be able to confirm it, but anyhow only in a couple of weeks.

Nadav