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

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

2014-11-26 19:01+0200, Nadav Amit:
> 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.

How many (V)CPUs were you using?
(We fail hard with logical destination x2APIC and 16+ VCPUs.)

>                                         All the core APICs are
> software-enabled. The expected behaviour is that the CPUs with x2APIC
> enabled would work in x2APIC mode.

(Nice, I bet that made some Intel designers happy.)

There shouldn't be any message conflict when using APIC IDs <255, so it
might be possible if the x2APIC isn't programmed to issue weird
messages, like physical to nonexistent APIC ID 0xff000000, which would
be also interpreted as xAPIC broadcast.

> 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?

I don't think this patch improves anything.
(Both behaviors are wrong and I think the current one is a bit less so.)

Our x2APIC implementation is a hack that allowed faster IPI thanks to 1
MSR exit instead of 2 MMIO ones.  No OS, that doesn't know KVM's
limitations, should have enabled it because we didn't emulate interrupt
remapping, which is an architectural requirement for x2APIC ...

And for more concrete points:
- Physical x2APIC isn't affected (only broadcast, which is incorrect
  either way)

- Logical x2APIC and xAPIC don't work at the same time
  - Btw. logical x2APIC isn't supposed to work (see KVM_X2APIC_CID_BITS)
  - Logical xAPIC is shifted incorrectly in x2APIC mode, so they are all
    going to be inaccessible (ldr = 0)
  - Our map isn't designed to allow x2APIC and xAPIC at the same time

- Your patch does not cover the case where sw-disabled x2APIC is
  "before" sw-enabled xAPIC, only if it is after.

> -- >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
> 
>