Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Tue, 10 May 2022 14:15:22 +0000
From:   Sean Christopherson <seanjc@google.com>
To:     Wanpeng Li <kernellwp@gmail.com>
Cc:     linux-kernel@vger.kernel.org, kvm@vger.kernel.org,
        Paolo Bonzini <pbonzini@redhat.com>,
        Vitaly Kuznetsov <vkuznets@redhat.com>,
        Wanpeng Li <wanpengli@tencent.com>,
        Jim Mattson <jmattson@google.com>,
        Joerg Roedel <joro@8bytes.org>
Subject: Re: [PATCH] KVM: LAPIC: Narrow down the timer fastpath to
 tscdeadline timer
Message-ID: <YnpzetR/B3nXVJxu@google.com>
References: <1651830457-11284-1-git-send-email-wanpengli@tencent.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <1651830457-11284-1-git-send-email-wanpengli@tencent.com>
Precedence: bulk

On Fri, May 06, 2022, Wanpeng Li wrote:
> From: Wanpeng Li <wanpengli@tencent.com>
> 
> The original timer fastpath is developed for tscdeadline timer, however,
> the apic timer periodic/oneshot mode which is emulated by vmx preemption
> timer goes to preemption timer vmexit fastpath quietly, let's leave the 
> complex recompute periodic timer's target expiration and restart apic 
> timer to the slowpath vm-exit. Narrow down the timer fastpath to tscdeadline
> timer mode.

Why?  I get that the original intention was only to handle deadline mode, but
that doesn't mean using it for other modes is inherently flawed/problematic.  KVM
also uses a fastpath for starting the deadline timer on MSR write, i.e. KVM eats
the cost of starting the timer in the fastpath no matter what, and
advance_periodic_target_expiration() isn't _that_ complex.

> Signed-off-by: Wanpeng Li <wanpengli@tencent.com>
> ---
>  arch/x86/kvm/lapic.c   |  6 ++++++
>  arch/x86/kvm/lapic.h   |  1 +
>  arch/x86/kvm/vmx/vmx.c | 14 +++++++++++---
>  3 files changed, 18 insertions(+), 3 deletions(-)
> 
> diff --git a/arch/x86/kvm/lapic.c b/arch/x86/kvm/lapic.c
> index 137c3a2f5180..3e6cb2bf56dc 100644
> --- a/arch/x86/kvm/lapic.c
> +++ b/arch/x86/kvm/lapic.c
> @@ -2459,6 +2459,12 @@ static bool lapic_is_periodic(struct kvm_lapic *apic)
>  	return apic_lvtt_period(apic);
>  }
>  
> +bool lapic_is_tscdeadline(struct kvm_lapic *apic)
> +{
> +	return apic_lvtt_tscdeadline(apic);
> +}
> +EXPORT_SYMBOL_GPL(lapic_is_tscdeadline);
> +
>  int apic_has_pending_timer(struct kvm_vcpu *vcpu)
>  {
>  	struct kvm_lapic *apic = vcpu->arch.apic;
> diff --git a/arch/x86/kvm/lapic.h b/arch/x86/kvm/lapic.h
> index 4e4f8a22754f..6e1b2f349237 100644
> --- a/arch/x86/kvm/lapic.h
> +++ b/arch/x86/kvm/lapic.h
> @@ -241,6 +241,7 @@ void kvm_lapic_expired_hv_timer(struct kvm_vcpu *vcpu);
>  bool kvm_lapic_hv_timer_in_use(struct kvm_vcpu *vcpu);
>  void kvm_lapic_restart_hv_timer(struct kvm_vcpu *vcpu);
>  bool kvm_can_use_hv_timer(struct kvm_vcpu *vcpu);
> +bool lapic_is_tscdeadline(struct kvm_lapic *apic);
>  
>  static inline enum lapic_mode kvm_apic_mode(u64 apic_base)
>  {
> diff --git a/arch/x86/kvm/vmx/vmx.c b/arch/x86/kvm/vmx/vmx.c
> index bb09fc9a7e55..2a8f4253df35 100644
> --- a/arch/x86/kvm/vmx/vmx.c
> +++ b/arch/x86/kvm/vmx/vmx.c
> @@ -5713,22 +5713,30 @@ static int handle_pml_full(struct kvm_vcpu *vcpu)
>  	return 1;
>  }
>  
> -static fastpath_t handle_fastpath_preemption_timer(struct kvm_vcpu *vcpu)
> +static bool __handle_preemption_timer(struct kvm_vcpu *vcpu)
>  {
>  	struct vcpu_vmx *vmx = to_vmx(vcpu);
>  
>  	if (!vmx->req_immediate_exit &&
>  	    !unlikely(vmx->loaded_vmcs->hv_timer_soft_disabled)) {
>  		kvm_lapic_expired_hv_timer(vcpu);
> -		return EXIT_FASTPATH_REENTER_GUEST;
> +		return true;
>  	}
>  
> +	return false;

It's a bit odd for the non-fastpath case, but I'd prefer to return fastpath_t
instead of a bool from the inner helper, e.g.

static fastpath_t __handle_preemption_timer(struct kvm_vcpu *vcpu)
{
	struct vcpu_vmx *vmx = to_vmx(vcpu);

	if (!vmx->req_immediate_exit &&
	    !unlikely(vmx->loaded_vmcs->hv_timer_soft_disabled)) {
		kvm_lapic_expired_hv_timer(vcpu);
		return EXIT_FASTPATH_REENTER_GUEST;
	}

	return EXIT_FASTPATH_NONE;
}

static fastpath_t handle_fastpath_preemption_timer(struct kvm_vcpu *vcpu)
{
	if (lapic_is_tscdeadline(vcpu->arch.apic)
		return __handle_preemption_timer(vcpu))

	return EXIT_FASTPATH_NONE;
}