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
[email protected]
https://lists.sourceforge.net/lists/listinfo/kvm-devel
