2012-05-04 11:45:02

by Srivatsa Vaddagiri

Subject: Re: [RFC PATCH v1 3/5] KVM: Add paravirt kvm_flush_tlb_others

* Nikunj A. Dadhania <[email protected]> [2012-04-27 21:54:37]:

> @@ -1549,6 +1549,11 @@ static void kvm_set_vcpu_state(struct kvm_vcpu *vcpu)
> return;
>
> vs->state = 1;
> + if (vs->flush_on_enter) {
> + kvm_mmu_flush_tlb(vcpu);
> + vs->flush_on_enter = 0;
> + }
> +
> kvm_write_guest_cached(vcpu->kvm, ghc, vs, 2*sizeof(__u32));

Reading flush_on_enter before writing ->state (=1) is racy afaics (and
may cause vcpu to miss a TLB flush request).

- vatsa

2012-05-07 03:29:08

by Nikunj A Dadhania

Subject: Re: [RFC PATCH v1 3/5] KVM: Add paravirt kvm_flush_tlb_others

On Fri, 4 May 2012 17:14:49 +0530, Srivatsa Vaddagiri <[email protected]> wrote:
> * Nikunj A. Dadhania <[email protected]> [2012-04-27 21:54:37]:
>
> > @@ -1549,6 +1549,11 @@ static void kvm_set_vcpu_state(struct kvm_vcpu *vcpu)
> > return;
> >
> > vs->state = 1;
> > + if (vs->flush_on_enter) {
> > + kvm_mmu_flush_tlb(vcpu);
> > + vs->flush_on_enter = 0;
> > + }
> > +
> > kvm_write_guest_cached(vcpu->kvm, ghc, vs, 2*sizeof(__u32));
>
> Reading flush_on_enter before writing ->state (=1) is racy afaics (and
> may cause vcpu to miss a TLB flush request).
>
Yes I see this with sysbench, here is what I have now, currently I have
tested it with sysbench(50 runs). Will fold this in my v2.

diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index 60546e9..b2ee9fd 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -1548,9 +1548,20 @@ static void kvm_set_vcpu_state(struct kvm_vcpu *vcpu)
if (!(vcpu->arch.v_state.msr_val & KVM_MSR_ENABLED))
return;

+ /*
+ * Let the guest know that we are online, make sure we do not
+ * overwrite flush_on_enter, just write the vs->state.
+ */
vs->state = 1;
- kvm_write_guest_cached(vcpu->kvm, ghc, vs, 2*sizeof(__u32));
+ kvm_write_guest_cached(vcpu->kvm, ghc, vs, 1*sizeof(__u32));
smp_wmb();
+ /*
+ * Guest might have seen us offline and would have set
+ * flush_on_enter.
+ */
+ kvm_read_guest_cached(vcpu->kvm, ghc, vs, 2*sizeof(__u32));
+ if (vs->flush_on_enter)
+ kvm_x86_ops->tlb_flush(vcpu);
}

static void kvm_clear_vcpu_state(struct kvm_vcpu *vcpu)


Nikunj