Re: Remove APIC lock

[Avi Kivity <avi-atKUWr5tajBWk0Htik3J/w@public.gmane.org>]

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/