diff mbox series

[v5,27/33] KVM: SVM: Require logical ID to be power-of-2 for AVIC entry

Message ID 20230106011306.85230-28-seanjc@google.com
State New
Headers
Series KVM: x86: AVIC and local APIC fixes+cleanups |

Commit Message

Sean Christopherson Jan. 6, 2023, 1:13 a.m. UTC 
  Do not modify AVIC's logical ID table if the logical ID portion of the
LDR is not a power-of-2, i.e. if the LDR has multiple bits set.  Taking
only the first bit means that KVM will fail to match MDAs that intersect
with "higher" bits in the "ID"

The "ID" acts as a bitmap, but is referred to as an ID because there's an
implicit, unenforced "requirement" that software only set one bit.  This
edge case is arguably out-of-spec behavior, but KVM cleanly handles it
in all other cases, e.g. the optimized logical map (and AVIC!) is also
disabled in this scenario.

Refactor the code to consolidate the checks, and so that the code looks
more like avic_kick_target_vcpus_fast().

Fixes: 18f40c53e10f ("svm: Add VMEXIT handlers for AVIC")
Cc: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>
Cc: Maxim Levitsky <mlevitsk@redhat.com>
Signed-off-by: Sean Christopherson <seanjc@google.com>
---
 arch/x86/kvm/svm/avic.c | 30 +++++++++++++++---------------
 1 file changed, 15 insertions(+), 15 deletions(-)

Comments

Maxim Levitsky Jan. 8, 2023, 3:20 p.m. UTC | #1 
On Fri, 2023-01-06 at 01:13 +0000, Sean Christopherson wrote:
> Do not modify AVIC's logical ID table if the logical ID portion of the
> LDR is not a power-of-2, i.e. if the LDR has multiple bits set.  Taking
> only the first bit means that KVM will fail to match MDAs that intersect
> with "higher" bits in the "ID"
> 
> The "ID" acts as a bitmap, but is referred to as an ID because there's an
> implicit, unenforced "requirement" that software only set one bit.  This
> edge case is arguably out-of-spec behavior, but KVM cleanly handles it
> in all other cases, e.g. the optimized logical map (and AVIC!) is also
> disabled in this scenario.
> 
> Refactor the code to consolidate the checks, and so that the code looks
> more like avic_kick_target_vcpus_fast().


Very minor nitpick about the subject, it feels like it just changes something
minor but it pretty much rewrites the avic_get_logical_id_entry.

When I bisected for the bug, this did confuse me a bit.

I don't have a good idea on how to call this patch so I won't object to the current
subject to stay as well.

Best regards,
	Maxim Levitsky


> 
> Fixes: 18f40c53e10f ("svm: Add VMEXIT handlers for AVIC")
> Cc: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>
> Cc: Maxim Levitsky <mlevitsk@redhat.com>
> Signed-off-by: Sean Christopherson <seanjc@google.com>
> ---
>  arch/x86/kvm/svm/avic.c | 30 +++++++++++++++---------------
>  1 file changed, 15 insertions(+), 15 deletions(-)
> 
> diff --git a/arch/x86/kvm/svm/avic.c b/arch/x86/kvm/svm/avic.c
> index 5b33f91b701c..eb979695e7d8 100644
> --- a/arch/x86/kvm/svm/avic.c
> +++ b/arch/x86/kvm/svm/avic.c
> @@ -513,26 +513,26 @@ unsigned long avic_vcpu_get_apicv_inhibit_reasons(struct kvm_vcpu *vcpu)
>  static u32 *avic_get_logical_id_entry(struct kvm_vcpu *vcpu, u32 ldr, bool flat)
>  {
>  	struct kvm_svm *kvm_svm = to_kvm_svm(vcpu->kvm);
> -	int index;
>  	u32 *logical_apic_id_table;
> -	int dlid = GET_APIC_LOGICAL_ID(ldr);
> +	u32 cluster, index;
>  
> -	if (!dlid)
> -		return NULL;
> +	ldr = GET_APIC_LOGICAL_ID(ldr);
>  
> -	if (flat) { /* flat */
> -		index = ffs(dlid) - 1;
> -		if (index > 7)
> +	if (flat) {
> +		cluster = 0;
> +	} else {
> +		cluster = (ldr >> 4);
> +		if (cluster >= 0xf)
>  			return NULL;
> -	} else { /* cluster */
> -		int cluster = (dlid & 0xf0) >> 4;
> -		int apic = ffs(dlid & 0x0f) - 1;
> -
> -		if ((apic < 0) || (apic > 7) ||
> -		    (cluster >= 0xf))
> -			return NULL;
> -		index = (cluster << 2) + apic;
> +		ldr &= 0xf;
>  	}
> +	if (!ldr || !is_power_of_2(ldr))
> +		return NULL;
> +
> +	index = __ffs(ldr);
> +	if (WARN_ON_ONCE(index > 7))
> +		return NULL;
> +	index += (cluster << 2);
>  
>  	logical_apic_id_table = (u32 *) page_address(kvm_svm->avic_logical_id_table_page);
>  

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

Best regards,
	Maxim Levitsky
diff mbox series

Patch

diff --git a/arch/x86/kvm/svm/avic.c b/arch/x86/kvm/svm/avic.c
index 5b33f91b701c..eb979695e7d8 100644
--- a/arch/x86/kvm/svm/avic.c
+++ b/arch/x86/kvm/svm/avic.c
@@ -513,26 +513,26 @@  unsigned long avic_vcpu_get_apicv_inhibit_reasons(struct kvm_vcpu *vcpu)
 static u32 *avic_get_logical_id_entry(struct kvm_vcpu *vcpu, u32 ldr, bool flat)
 {
 	struct kvm_svm *kvm_svm = to_kvm_svm(vcpu->kvm);
-	int index;
 	u32 *logical_apic_id_table;
-	int dlid = GET_APIC_LOGICAL_ID(ldr);
+	u32 cluster, index;
 
-	if (!dlid)
-		return NULL;
+	ldr = GET_APIC_LOGICAL_ID(ldr);
 
-	if (flat) { /* flat */
-		index = ffs(dlid) - 1;
-		if (index > 7)
+	if (flat) {
+		cluster = 0;
+	} else {
+		cluster = (ldr >> 4);
+		if (cluster >= 0xf)
 			return NULL;
-	} else { /* cluster */
-		int cluster = (dlid & 0xf0) >> 4;
-		int apic = ffs(dlid & 0x0f) - 1;
-
-		if ((apic < 0) || (apic > 7) ||
-		    (cluster >= 0xf))
-			return NULL;
-		index = (cluster << 2) + apic;
+		ldr &= 0xf;
 	}
+	if (!ldr || !is_power_of_2(ldr))
+		return NULL;
+
+	index = __ffs(ldr);
+	if (WARN_ON_ONCE(index > 7))
+		return NULL;
+	index += (cluster << 2);
 
 	logical_apic_id_table = (u32 *) page_address(kvm_svm->avic_logical_id_table_page);