[PATCH 12/19] KVM: x86: Disable APIC logical map if logical ID covers multiple MDAs

Sean Christopherson posted 19 patches 3 years, 7 months ago
There is a newer version of this series
[PATCH 12/19] KVM: x86: Disable APIC logical map if logical ID covers multiple MDAs
Posted by Sean Christopherson 3 years, 7 months ago
Disable the optimized APIC logical map if a logical ID covers multiple
MDAs, i.e. if a vCPU has multiple bits set in its ID.  In logical mode,
events match if "ID & MDA != 0", i.e. creating an entry for only the
first bit can cause interrupts to be missed.

Note, creating an entry for every bit is also wrong as KVM would generate
IPIs for every matching bit.  It would be possible to teach KVM to play
nice with this edge case, but it is very much an edge case and probably
not used in any real world OS, i.e. it's not worth optimizing.

Use an impossible value for the "mode" to effectively designate that it's
disabled.  Don't bother adding a dedicated "invalid" value, the mode
handling will be cleaned up in the future and it would take just as much
effort to explain what value is "safe" at this time.

Fixes: 1e08ec4a130e ("KVM: optimize apic interrupt delivery")
Signed-off-by: Sean Christopherson <seanjc@google.com>
---
 arch/x86/kvm/lapic.c | 11 +++++++++--
 1 file changed, 9 insertions(+), 2 deletions(-)

diff --git a/arch/x86/kvm/lapic.c b/arch/x86/kvm/lapic.c
index 9dda989a1cf0..82278acae95b 100644
--- a/arch/x86/kvm/lapic.c
+++ b/arch/x86/kvm/lapic.c
@@ -300,8 +300,15 @@ void kvm_recalculate_apic_map(struct kvm *kvm)
 		if (!kvm_apic_map_get_logical_dest(new, ldr, &cluster, &mask))
 			continue;
 
-		if (mask)
-			cluster[ffs(mask) - 1] = apic;
+		if (!mask)
+			continue;
+
+		if (!is_power_of_2(mask)) {
+			new->mode = KVM_APIC_MODE_XAPIC_FLAT |
+				    KVM_APIC_MODE_XAPIC_CLUSTER;
+			continue;
+		}
+		cluster[ffs(mask) - 1] = apic;
 	}
 out:
 	old = rcu_dereference_protected(kvm->arch.apic_map,
-- 
2.37.2.672.g94769d06f0-goog
Re: [PATCH 12/19] KVM: x86: Disable APIC logical map if logical ID covers multiple MDAs
Posted by Maxim Levitsky 3 years, 7 months ago
On Wed, 2022-08-31 at 00:34 +0000, Sean Christopherson wrote:
> Disable the optimized APIC logical map if a logical ID covers multiple
> MDAs, i.e. if a vCPU has multiple bits set in its ID.  In logical mode,
> events match if "ID & MDA != 0", i.e. creating an entry for only the
> first bit can cause interrupts to be missed.
> 
> Note, creating an entry for every bit is also wrong as KVM would generate
> IPIs for every matching bit.  It would be possible to teach KVM to play
> nice with this edge case, but it is very much an edge case and probably
> not used in any real world OS, i.e. it's not worth optimizing.
> 
> Use an impossible value for the "mode" to effectively designate that it's
> disabled.  Don't bother adding a dedicated "invalid" value, the mode
> handling will be cleaned up in the future and it would take just as much
> effort to explain what value is "safe" at this time.
> 
> Fixes: 1e08ec4a130e ("KVM: optimize apic interrupt delivery")
> Signed-off-by: Sean Christopherson <seanjc@google.com>
> ---
>  arch/x86/kvm/lapic.c | 11 +++++++++--
>  1 file changed, 9 insertions(+), 2 deletions(-)
> 
> diff --git a/arch/x86/kvm/lapic.c b/arch/x86/kvm/lapic.c
> index 9dda989a1cf0..82278acae95b 100644
> --- a/arch/x86/kvm/lapic.c
> +++ b/arch/x86/kvm/lapic.c
> @@ -300,8 +300,15 @@ void kvm_recalculate_apic_map(struct kvm *kvm)
>  		if (!kvm_apic_map_get_logical_dest(new, ldr, &cluster, &mask))
>  			continue;
>  
> -		if (mask)
> -			cluster[ffs(mask) - 1] = apic;
> +		if (!mask)
> +			continue;
> +
> +		if (!is_power_of_2(mask)) {
> +			new->mode = KVM_APIC_MODE_XAPIC_FLAT |
> +				    KVM_APIC_MODE_XAPIC_CLUSTER;
> +			continue;
> +		}
> +		cluster[ffs(mask) - 1] = apic;
>  	}
>  out:
>  	old = rcu_dereference_protected(kvm->arch.apic_map,


I was about to complain about the abuse of the invalid mode,
but I see that this is fixed in later patch as it is said in the commit
description, so no complains.

Reviewed-by: Maxim Levitsky <mlevitsk@redhat.com>

Best regards,
	Maxim Levitsky