* Re: [PATCH] KVM: SVM: move dest calculation out of loop
2022-08-26 5:43 [PATCH] KVM: SVM: move dest calculation out of loop Li RongQing
@ 2022-08-26 15:35 ` Sean Christopherson
0 siblings, 0 replies; 2+ messages in thread
From: Sean Christopherson @ 2022-08-26 15:35 UTC (permalink / raw)
To: Li RongQing; +Cc: x86, kvm
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
--
^ permalink raw reply related [flat|nested] 2+ messages in thread