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. When guest change/disable APIC time configuration, the hrtimer must be canceled first so that no race with old hrtimer. > > > >> >> > > 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. > - vcpu enters guest mode on cpu 1 > - cpu 0 calls apic_timer_fn The timer is either already canceled or be done before above migration completion. > > In this case, there will be no wakeup. So I think you do need to call > kvm_vcpu_kick() which will usually do nothing. > > We also need to make sure all the non atomic code in __apic_timer_fn() > is executed in process context (it can use the pending count to > decide how much to add). I think today it is all atomic ops there. maybe I missed something. > > So I think there are three separate issues here: > - hrtimer migration: it helps performance, but doesn't help locking > - changing __apic_timer_fn() to only do atomic operations, and do the > nonatomic operations in process context under vcpu->mutex - remove > the apic lock > thx,eddie ------------------------------------------------------------------------- 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
