Re: [PATCH v3] KVM: LAPIC: Fix lapic timer injection delay

[Paolo Bonzini <pbonzini@redhat.com>]

On 29/06/2017 05:42, Wanpeng Li wrote:
> So how about something like this?
> 
> diff --git a/arch/x86/kvm/lapic.c b/arch/x86/kvm/lapic.c
> index d24c874..900ce86 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);
> +
> +            /* 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 && (apic_lvtt_oneshot(apic) ||
> apic_lvtt_tscdeadline(apic)))
> +                apic_timer_expired(apic);
> +        }
> +    }
> 
> -    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);
> +    if (need_cancel)
> +        cancel_hv_timer(apic);
> 
> -        /* In case the sw timer triggered in the window */
> -        if (atomic_read(&apic->lapic_timer.pending) &&
> -            !apic_lvtt_period(apic))
> -            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;
> diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c
> index 4f616db..0a0e696 100644
> --- a/arch/x86/kvm/vmx.c
> +++ b/arch/x86/kvm/vmx.c
> @@ -11125,6 +11125,9 @@ static int vmx_set_hv_timer(struct kvm_vcpu
> *vcpu, u64 guest_deadline_tsc)
>      u64 guest_tscl = kvm_read_l1_tsc(vcpu, tscl);
>      u64 delta_tsc = max(guest_deadline_tsc, guest_tscl) - guest_tscl;
> 
> +    if (delta_tsc == 0)
> +        return 1;

You still need to enable the preemption timer even if you return 1, so
in lapic.c it becomes

	if (!apic_lvtt_period(apic)) {
		if (r)
			apic_timer_expired(apic);
		if (atomic_read(&apic->lapic_timer.pending))
			need_cancel = true;
	}


Otherwise it looks good.  Thanks for your persistence. :)

Paolo

>      /* Convert to host delta tsc if tsc scaling is enabled */
>      if (vcpu->arch.tsc_scaling_ratio != kvm_default_tsc_scaling_ratio &&
>              u64_shl_div_u64(delta_tsc,
>