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?
Regards,
Wanpeng Li
