| Message ID | 20211009021236.4122790-3-seanjc@google.com (mailing list archive) |
|---|---|
| State | Not Applicable |
| Headers | show |
| Series | KVM: Halt-polling and x86 APICv overhaul | expand |
On 09/10/21 04:11, Sean Christopherson wrote: > + * point, which could result in signalling the wrong/previous > + * pCPU. But if that happens the vCPU is guaranteed to do a > + * VMRUN (after being migrated) and thus will process pending > + * interrupts, i.e. a doorbell is not needed (and the spurious) ... one is harmless, I suppose. Paolo
On Fri, 2021-10-08 at 19:11 -0700, Sean Christopherson wrote: > Ensure vcpu->cpu is read once when signalling the AVIC doorbell. If the > compiler rereads the field and the vCPU is migrated between the check and > writing the doorbell, KVM would signal the wrong physical CPU. Since vcpu->cpu can change any moment anyway, adding READ_ONCE I think can't really fix anything but I do agree that it makes this more readable. Reviewed-by: Maxim Levitsky <mlevitsk@redhat.com> > > Functionally, signalling the wrong CPU in this case is not an issue as > task migration means the vCPU has exited and will pick up any pending > interrupts on the next VMRUN. Add the READ_ONCE() purely to clean up the > code. > > Opportunistically add a comment explaining the task migration behavior, > and rename cpuid=>cpu to avoid conflating the CPU number with KVM's more > common usage of CPUID. > > Signed-off-by: Sean Christopherson <seanjc@google.com> > --- > arch/x86/kvm/svm/avic.c | 13 ++++++++++--- > 1 file changed, 10 insertions(+), 3 deletions(-) > > diff --git a/arch/x86/kvm/svm/avic.c b/arch/x86/kvm/svm/avic.c > index 8052d92069e0..208c5c71e827 100644 > --- a/arch/x86/kvm/svm/avic.c > +++ b/arch/x86/kvm/svm/avic.c > @@ -675,10 +675,17 @@ int svm_deliver_avic_intr(struct kvm_vcpu *vcpu, int vec) > smp_mb__after_atomic(); > > if (avic_vcpu_is_running(vcpu)) { > - int cpuid = vcpu->cpu; > + int cpu = READ_ONCE(vcpu->cpu); > > - if (cpuid != get_cpu()) > - wrmsrl(SVM_AVIC_DOORBELL, kvm_cpu_get_apicid(cpuid)); > + /* > + * Note, the vCPU could get migrated to a different pCPU at any > + * point, which could result in signalling the wrong/previous > + * pCPU. But if that happens the vCPU is guaranteed to do a > + * VMRUN (after being migrated) and thus will process pending > + * interrupts, i.e. a doorbell is not needed (and the spurious) > + */ > + if (cpu != get_cpu()) > + wrmsrl(SVM_AVIC_DOORBELL, kvm_cpu_get_apicid(cpu)); > put_cpu(); > } else > kvm_vcpu_wake_up(vcpu);
diff --git a/arch/x86/kvm/svm/avic.c b/arch/x86/kvm/svm/avic.c index 8052d92069e0..208c5c71e827 100644 --- a/arch/x86/kvm/svm/avic.c +++ b/arch/x86/kvm/svm/avic.c @@ -675,10 +675,17 @@ int svm_deliver_avic_intr(struct kvm_vcpu *vcpu, int vec) smp_mb__after_atomic(); if (avic_vcpu_is_running(vcpu)) { - int cpuid = vcpu->cpu; + int cpu = READ_ONCE(vcpu->cpu); - if (cpuid != get_cpu()) - wrmsrl(SVM_AVIC_DOORBELL, kvm_cpu_get_apicid(cpuid)); + /* + * Note, the vCPU could get migrated to a different pCPU at any + * point, which could result in signalling the wrong/previous + * pCPU. But if that happens the vCPU is guaranteed to do a + * VMRUN (after being migrated) and thus will process pending + * interrupts, i.e. a doorbell is not needed (and the spurious) + */ + if (cpu != get_cpu()) + wrmsrl(SVM_AVIC_DOORBELL, kvm_cpu_get_apicid(cpu)); put_cpu(); } else kvm_vcpu_wake_up(vcpu);
Ensure vcpu->cpu is read once when signalling the AVIC doorbell. If the compiler rereads the field and the vCPU is migrated between the check and writing the doorbell, KVM would signal the wrong physical CPU. Functionally, signalling the wrong CPU in this case is not an issue as task migration means the vCPU has exited and will pick up any pending interrupts on the next VMRUN. Add the READ_ONCE() purely to clean up the code. Opportunistically add a comment explaining the task migration behavior, and rename cpuid=>cpu to avoid conflating the CPU number with KVM's more common usage of CPUID. Signed-off-by: Sean Christopherson <seanjc@google.com> --- arch/x86/kvm/svm/avic.c | 13 ++++++++++--- 1 file changed, 10 insertions(+), 3 deletions(-)