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

[Paolo Bonzini <pbonzini@redhat.com>]

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.

Paolo