On 28/06/2017 16:27, Wanpeng Li wrote:
> 2017-06-28 20:10 GMT+08:00 Paolo Bonzini <pbonzini@xxxxxxxxxx>:
>>
>>
>> On 28/06/2017 03:29, Wanpeng Li wrote:
>>> u64 tscdeadline = apic->lapic_timer.tscdeadline;
>>> + int ret = 0;
>>>
>>> if ((atomic_read(&apic->lapic_timer.pending) &&
>>> !apic_lvtt_period(apic)) ||
>>> - kvm_x86_ops->set_hv_timer(apic->vcpu, tscdeadline)) {
>>> + (ret = kvm_x86_ops->set_hv_timer(apic->vcpu, tscdeadline))) {
>>> if (apic->lapic_timer.hv_timer_in_use)
>>> cancel_hv_timer(apic);
>>> + if (ret == 1) {
>>> + apic_timer_expired(apic);
>>> + return true;
>>> + }
>>
>> The preemption timer can also be used for modes other than TSC deadline.
>>
>> In periodic mode, your patch would miss a call to
>> advance_periodic_target_expiration, which is only called by
>> kvm_lapic_expired_hv_timer.
>>
>> You could use something like this:
>>
>> diff --git a/arch/x86/kvm/lapic.c b/arch/x86/kvm/lapic.c
>> index d24c8742d9b0..15b751aa7625 100644
>> --- a/arch/x86/kvm/lapic.c
>> +++ b/arch/x86/kvm/lapic.c
>> @@ -1504,21 +1504,26 @@ static void cancel_hv_timer(struct kvm_lapic *apic)
>> static bool start_hv_timer(struct kvm_lapic *apic)
>> {
>> u64 tscdeadline = apic->lapic_timer.tscdeadline;
>> + bool need_cancel = apic->lapic_timer.hv_timer_in_use;
>> + if (!atomic_read(&apic->lapic_timer.pending) || apic_lvtt_period(apic)) {
>> + int r = kvm_x86_ops->set_hv_timer(apic->vcpu, tscdeadline);
>> + if (r >= 0) {
>> + need_cancel = false;
>> + apic->lapic_timer.hv_timer_in_use = true;
>> + hrtimer_cancel(&apic->lapic_timer.timer);
>>
>> - if ((atomic_read(&apic->lapic_timer.pending) &&
>> - !apic_lvtt_period(apic)) ||
>> - kvm_x86_ops->set_hv_timer(apic->vcpu, tscdeadline)) {
>> - if (apic->lapic_timer.hv_timer_in_use)
>> - cancel_hv_timer(apic);
>> - } else {
>> - apic->lapic_timer.hv_timer_in_use = true;
>> - hrtimer_cancel(&apic->lapic_timer.timer);
>> -
>> - /* In case the sw timer triggered in the window */
>> - if (atomic_read(&apic->lapic_timer.pending) &&
>> - !apic_lvtt_period(apic))
>> - cancel_hv_timer(apic);
>> + /* In case the sw timer triggered in the window */
>> + if (atomic_read(&apic->lapic_timer.pending) &&
>> + !apic_lvtt_period(apic))
>> + need_cancel = true;
>> + else if (r)
>> + kvm_lapic_expired_hv_timer(vcpu);
>> + }
>> }
>> +
>> + if (need_cancel)
>> + cancel_hv_timer(apic);
>> +
>> trace_kvm_hv_timer_state(apic->vcpu->vcpu_id,
>> apic->lapic_timer.hv_timer_in_use);
>> return apic->lapic_timer.hv_timer_in_use;
>>
>> but I'm afraid of introducing a mutual recursion between
>> start_hv_timer and kvm_lapic_expired_hv_timer.
>
> We can just handle the apic timer oneshot/tscdeadline mode instead of
> periodic mode just like which is emulated by hrtimer to avoid the
> mutual recusion, what do you think?
In that case, set_hv_timer should probably always enable the preemption
timer. You can then cancel it if it returns 1 _and_ the APIC timer's
mode is oneshot/tscdeadline.
Paolo
