Re: [PATCH 5/5] KVM: LAPIC: add APIC Timer periodic/oneshot mode VMX preemption timer support

["Radim Krčmář" <rkrcmar@redhat.com>]

2016-10-17 15:45+0800, Wanpeng Li:
> From: Wanpeng Li <wanpeng.li@hotmail.com>
> 
> Most windows guests still utilize APIC Timer periodic/oneshot mode
> instead of tsc-deadline mode, and the APIC Timer periodic/oneshot
> mode are still emulated by high overhead hrtimer on host. This patch
> converts the expected expire time of the periodic/oneshot mode to
> guest deadline tsc in order to leverage VMX preemption timer logic
> for APIC Timer tsc-deadline mode. After each preemption timer vmexit
> preemption timer is restarted to emulate LVTT current-count register
> is automatically reloaded from the initial-count register when the
> count reaches 0. This patch reduces ~3800 cycles for each APIC Timer
> periodic mode operation virtualization.
> 
> Cc: Paolo Bonzini <pbonzini@redhat.com>
> Cc: Radim Krčmář <rkrcmar@redhat.com>
> Cc: Yunhong Jiang <yunhong.jiang@intel.com>
> Signed-off-by: Wanpeng Li <wanpeng.li@hotmail.com>
> ---
> diff --git a/arch/x86/kvm/lapic.c b/arch/x86/kvm/lapic.c
> @@ -1090,7 +1090,7 @@ static void apic_send_ipi(struct kvm_lapic *apic)
>  
>  static u32 apic_get_tmcct(struct kvm_lapic *apic)
>  {
> -	ktime_t remaining;
> +	ktime_t remaining, now;
>  	s64 ns;
>  	u32 tmcct;
>  
> @@ -1101,7 +1101,8 @@ static u32 apic_get_tmcct(struct kvm_lapic *apic)
>  		apic->lapic_timer.period == 0)
>  		return 0;
>  
> -	remaining = hrtimer_get_remaining(&apic->lapic_timer.timer);
> +	now = apic->lapic_timer.timer.base->get_time();
> +	remaining = ktime_sub(apic->lapic_timer.target_expiration, now);

Periodic timer does not advance apic->lapic_timer.target_expiration,
when rearming the hrtimer, so this would incorrectly return 0 in
subsequent periods.

>  	if (ktime_to_ns(remaining) < 0)
>  		remaining = ktime_set(0, 0);
>  
> @@ -1438,14 +1447,42 @@ static bool start_hv_timer(struct kvm_lapic *apic)
>  	return apic->lapic_timer.hv_timer_in_use;
>  }
>  
> +void kvm_lapic_expired_hv_timer(struct kvm_vcpu *vcpu)
> +{
> +	struct kvm_lapic *apic = vcpu->arch.apic;
> +
> +	WARN_ON(!apic->lapic_timer.hv_timer_in_use);
> +	WARN_ON(swait_active(&vcpu->wq));
> +	cancel_hv_timer(apic);
> +	apic_timer_expired(apic);
> +
> +	if (apic_lvtt_period(apic) &&
> +		set_target_expiration(apic) &&
> +		!start_hv_timer(apic))
> +		start_sw_period(apic);
> +}
> +EXPORT_SYMBOL_GPL(kvm_lapic_expired_hv_timer);
> +
>  void kvm_lapic_switch_to_hv_timer(struct kvm_vcpu *vcpu)
>  {
>  	struct kvm_lapic *apic = vcpu->arch.apic;
>  
>  	WARN_ON(apic->lapic_timer.hv_timer_in_use);
>  
> -	if (apic_lvtt_tscdeadline(apic))
> -		start_hv_timer(apic);
> +	if (apic_lvtt_period(apic) || apic_lvtt_oneshot(apic)) {
> +		ktime_t remaining, now;
> +		u64 tscl = rdtsc();
> +
> +		now = apic->lapic_timer.timer.base->get_time();
> +		remaining = hrtimer_get_remaining(&apic->lapic_timer.timer);
> +		if (ktime_to_ns(remaining) < 0)
> +			remaining = ktime_set(0, 0);
> +
> +		apic->lapic_timer.tscdeadline = kvm_read_l1_tsc(apic->vcpu, tscl) +
> +			nsec_to_cycles(apic->vcpu, ktime_to_ns(remaining));
> +		apic->lapic_timer.target_expiration = ktime_add_ns(now, ktime_to_ns(remaining));

For oneshot timer, there is no need to hrtimer_get_remaining(), because
apic->lapic_timer.tscdeadline and apic->lapic_timer.target_expiration
are already correct, so we could just use them.

The same could be true for the periodic timer as well, but
apic->lapic_timer.target_expiration nor apic->lapic_timer.tscdeadline is
advanced in apic_timer_fn(), so they are soon incorrect.

I think it would be better to add a function to advance the periodic
timer and use it in kvm_lapic_expired_hv_timer() and in apic_timer_fn().

The function can be simpler than set_target_expiration(), because it
just adds the period to an existing timer.  Periodic timer will also be
better then, because the period will not depend on KVM's latency when
rearming.

> +	}
> +	start_hv_timer(apic);
>  }
>  EXPORT_SYMBOL_GPL(kvm_lapic_switch_to_hv_timer);
>  
> @@ -1462,7 +1499,10 @@ void kvm_lapic_switch_to_sw_timer(struct kvm_vcpu *vcpu)
>  	if (atomic_read(&apic->lapic_timer.pending))
>  		return;
>  
> -	start_sw_tscdeadline(apic);
> +	if (apic_lvtt_period(apic) || apic_lvtt_oneshot(apic))
> +		start_sw_period(apic);
> +	else if (apic_lvtt_tscdeadline(apic))
> +		start_sw_tscdeadline(apic);
>  }
>  EXPORT_SYMBOL_GPL(kvm_lapic_switch_to_sw_timer);
>  
> @@ -1470,9 +1510,11 @@ static void start_apic_timer(struct kvm_lapic *apic)
>  {
>  	atomic_set(&apic->lapic_timer.pending, 0);
>  
> -	if (apic_lvtt_period(apic) || apic_lvtt_oneshot(apic))
> -		start_sw_period(apic);
> -	else if (apic_lvtt_tscdeadline(apic)) {
> +	if (apic_lvtt_period(apic) || apic_lvtt_oneshot(apic)) {
> +		if (set_target_expiration(apic) &&
> +			!(kvm_x86_ops->set_hv_timer && start_hv_timer(apic)))
> +			start_sw_period(apic);
> +	} else if (apic_lvtt_tscdeadline(apic)) {
>  		if (!(kvm_x86_ops->set_hv_timer && start_hv_timer(apic)))
>  			start_sw_tscdeadline(apic);
>  	}
> @@ -2005,8 +2047,11 @@ void kvm_inject_apic_timer_irqs(struct kvm_vcpu *vcpu)
>  
>  	if (atomic_read(&apic->lapic_timer.pending) > 0) {
>  		kvm_apic_local_deliver(apic, APIC_LVTT);
> -		if (apic_lvtt_tscdeadline(apic))
> +		if (!(apic_lvtt_period(apic) &&
> +			kvm_lapic_hv_timer_in_use(vcpu))) {

This would zero apic->lapic_timer.target_expiration of
apic_lvtt_period() when !kvm_lapic_hv_timer_in_use().

I think we don't want to ever do that, so we want

  		if (!(apic_lvtt_period(apic)) {

or maybe even better

  		if (apic_lvtt_tscdeadline(apic))
  			apic->lapic_timer.tscdeadline = 0;

  		if (apic_lvtt_oneshot(apic)) {
  			apic->lapic_timer.tscdeadline = 0;
  			apic->lapic_timer.target_expiration = ktime_set(0, 0);
  		}

>  			apic->lapic_timer.tscdeadline = 0;
> +			apic->lapic_timer.target_expiration = ktime_set(0, 0);
> +		}
>  		atomic_set(&apic->lapic_timer.pending, 0);
>  	}
>  }