Dong, Eddie wrote: > Avi Kivity wrote: > >>> Just noticed it is changed to mutex, but seems same here :-) >>> If the process is switched to other task, it is OK since it won't >>> access local APIC. Current VP access to APIC will take the mutex >>> first (see below). Or you are talking other corner case? >>> >>> >>> >> apic access from process context is protected by kvm->lock, but apic >> access from hrtimer is not. Consider this scenario: >> >> - guest accesses apic >> - apic code starts modifying apic data >> <preemption> >> - timer fires >> - apic_timer_fn() corrupts apic data >> >> (I'm not even sure preemption is required here) >> >> I think that in Xen this can't happen because is is not preemptible >> and timers are processed when exiting back to the guest. >> > > For this situation, even without preemption, the problem is still there. > But maybe you are refering the old code, the latest code is already > preemption free since the apic_timer_fn didn't change any APIC > state. It only increase apic->timer.pending. > >
I see the following: > static int __apic_timer_fn(struct kvm_lapic *apic) > { > int result = 0; > wait_queue_head_t *q = &apic->vcpu->wq; > > atomic_inc(&apic->timer.pending); > if (waitqueue_active(q)) > wake_up_interruptible(q); > if (apic_lvtt_period(apic)) { > result = 1; > apic->timer.dev.expires = ktime_add_ns( > apic->timer.dev.expires, > apic->timer.period); > } > return result; > } So, timer.dev.expires is protected by hrtimer internal locking? Tricky, but it should work. >>> >> The apic can be protected by vcpu->mutex, platform-wide things (pic, >> ioapic) should be protected by kvm->lock. This will work if >> we move all >> apic processing to process context like I proposed in a previous mail. >> >> >> >>> In this patch since hrtimer always run in same pCPU with guest VP >>> (when VP is active), each time when hrtime fires (comes from a >>> hardware IRQ), it already VM Exit to kernel (similar function with >>> kvm_vcpu_kick but no need to explicitly call it) and then we do IRQ >>> injection at vmx_intr_assist time. >>> >>> >> Yes, the two solutions are very similar. But I think mine protects >> against a race: >> >> - scheduler starts migrating vcpu from cpu 0 to cpu 1 >> - hrtimer fires on cpu 0, but apic_timer_fn not called yet >> - vcpu on cpu 1 migrates the hrtimer >> > > When CPU 1 do hrtimer migration, hrtimer_cancel will wait for > an in-flying timer be completed and then remove it. see > hrtimer_try_to_cancel. > Okay. I'm satisfied that it's safe now. I'll add some comments later and commit. -- Any sufficiently difficult bug is indistinguishable from a feature. ------------------------------------------------------------------------- This SF.net email is sponsored by: Splunk Inc. Still grepping through log files to find problems? Stop. Now Search log events and configuration files using AJAX and a browser. Download your FREE copy of Splunk now >> http://get.splunk.com/ _______________________________________________ kvm-devel mailing list kvm-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/kvm-devel