Re: [PATCH] KVM: SVM: move dest calculation out of loop

[Sean Christopherson <seanjc@google.com>]

On Fri, Aug 26, 2022, Li RongQing wrote:
> There is no need to calculate dest in each vcpu iteration
> since dest is not change
> 
> Signed-off-by: Li RongQing <lirongqing@baidu.com>
> ---
>  arch/x86/kvm/svm/avic.c | 13 +++++++------
>  1 file changed, 7 insertions(+), 6 deletions(-)
> 
> diff --git a/arch/x86/kvm/svm/avic.c b/arch/x86/kvm/svm/avic.c
> index 6919dee..087c073 100644
> --- a/arch/x86/kvm/svm/avic.c
> +++ b/arch/x86/kvm/svm/avic.c
> @@ -451,6 +451,7 @@ static int avic_kick_target_vcpus_fast(struct kvm *kvm, struct kvm_lapic *source
>  static void avic_kick_target_vcpus(struct kvm *kvm, struct kvm_lapic *source,
>  				   u32 icrl, u32 icrh, u32 index)
>  {
> +	u32 dest;
>  	unsigned long i;
>  	struct kvm_vcpu *vcpu;
>  
> @@ -465,13 +466,13 @@ static void avic_kick_target_vcpus(struct kvm *kvm, struct kvm_lapic *source,
>  	 * vCPUs that were in guest at the time of the IPI, and vCPUs that have
>  	 * since entered the guest will have processed pending IRQs at VMRUN.
>  	 */
> -	kvm_for_each_vcpu(i, vcpu, kvm) {
> -		u32 dest;
>  
> -		if (apic_x2apic_mode(vcpu->arch.apic))
> -			dest = icrh;
> -		else
> -			dest = GET_XAPIC_DEST_FIELD(icrh);
> +	if (apic_x2apic_mode(vcpu->arch.apic))

Please try to actually test patches before posting.  "vcpu" is quite clearly accessed
uninitialized.  gcc isn't smart enough to warn, but clang is.  I realize that testing
AVIC is more difficult than it should be, but it's not prohitively difficult.

arch/x86/kvm/svm/avic.c:470:23: error: variable 'vcpu' is uninitialized when used here [-Werror,-Wuninitialized]
        if (apic_x2apic_mode(vcpu->arch.apic))
                             ^~~~
arch/x86/kvm/svm/avic.c:456:23: note: initialize the variable 'vcpu' to silence this warning
        struct kvm_vcpu *vcpu;
                             ^
                              = NULL


That said, there is actually a functional bug here.  "dest" needs to be computed
using the source x2APIC status.

I'll send a small series, there are more cleanups than can be done by moving the
dissection of ichr/ichl into avic_kick_target_vcpus() instead of duplicating the
code in avic_kick_target_vcpus_fast().

--
From: Sean Christopherson <seanjc@google.com>
Date: Fri, 26 Aug 2022 08:30:57 -0700
Subject: [PATCH] KVM: SVM: Compute dest based on sender's x2APIC status for
 AVIC kick

Compute the destination from ICRH using the sender's x2APIC status, not
each (potential) target's x2APIC status.

Fixes: c514d3a348ac ("KVM: SVM: Update avic_kick_target_vcpus to support 32-bit APIC ID")
Cc: Li RongQing <lirongqing@baidu.com>
Signed-off-by: Sean Christopherson <seanjc@google.com>
---
 arch/x86/kvm/svm/avic.c | 8 +-------
 1 file changed, 1 insertion(+), 7 deletions(-)

diff --git a/arch/x86/kvm/svm/avic.c b/arch/x86/kvm/svm/avic.c
index 6919dee69f18..623431289d88 100644
--- a/arch/x86/kvm/svm/avic.c
+++ b/arch/x86/kvm/svm/avic.c
@@ -451,6 +451,7 @@ static int avic_kick_target_vcpus_fast(struct kvm *kvm, struct kvm_lapic *source
 static void avic_kick_target_vcpus(struct kvm *kvm, struct kvm_lapic *source,
 				   u32 icrl, u32 icrh, u32 index)
 {
+	u32 dest = apic_x2apic_mode(source) ? icrh : GET_XAPIC_DEST_FIELD(icrh);
 	unsigned long i;
 	struct kvm_vcpu *vcpu;

@@ -466,13 +467,6 @@ static void avic_kick_target_vcpus(struct kvm *kvm, struct kvm_lapic *source,
 	 * since entered the guest will have processed pending IRQs at VMRUN.
 	 */
 	kvm_for_each_vcpu(i, vcpu, kvm) {
-		u32 dest;
-
-		if (apic_x2apic_mode(vcpu->arch.apic))
-			dest = icrh;
-		else
-			dest = GET_XAPIC_DEST_FIELD(icrh);
-
 		if (kvm_apic_match_dest(vcpu, source, icrl & APIC_SHORT_MASK,
 					dest, icrl & APIC_DEST_MASK)) {
 			vcpu->arch.apic->irr_pending = true;

base-commit: 372d07084593dc7a399bf9bee815711b1fb1bcf2
--