Re: [PATCH][RFC] KVM: x86: Don't reset deadline to period when timer is in one shot mode

[Sean Christopherson <seanjc@google.com>]

+Jim, Peter, and Wanpeng

On Wed, Oct 12, 2022, Li RongQing wrote:
> In one-shot mode, the APIC timer stops counting when the timer
> reaches zero, so don't reset deadline to period for one shot mode
> 
> Signed-off-by: Li RongQing <lirongqing@baidu.com>
> ---
>  arch/x86/kvm/lapic.c | 8 ++++++--
>  1 file changed, 6 insertions(+), 2 deletions(-)
> 
> diff --git a/arch/x86/kvm/lapic.c b/arch/x86/kvm/lapic.c
> index 9dda989..bf39027 100644
> --- a/arch/x86/kvm/lapic.c
> +++ b/arch/x86/kvm/lapic.c
> @@ -1840,8 +1840,12 @@ static bool set_target_expiration(struct kvm_lapic *apic, u32 count_reg)
>  		if (unlikely(count_reg != APIC_TMICT)) {
>  			deadline = tmict_to_ns(apic,
>  				     kvm_lapic_get_reg(apic, count_reg));
> -			if (unlikely(deadline <= 0))
> -				deadline = apic->lapic_timer.period;
> +			if (unlikely(deadline <= 0)) {
> +				if (apic_lvtt_period(apic))
> +					deadline = apic->lapic_timer.period;
> +				else
> +					deadline = 0;
> +			}

This is not the standard "count has reached zero" path, it's the "vCPU is migrated
and the timer needs to be resumed on the destination" path.  Zeroing the deadline
here will not squash the timer, IIUC it will cause the timer to immediately fire.

That said, I think the patch is actually correct even though the shortlog+changelog
are wrong.  If the timer expired while the vCPU was migrated, KVM _should_ fire
the timer ASAP.  AFAICT, nothing else in KVM will detect the expired timer, e.g.
if the timer expired on the source, apic_get_tmcct() on the source should have
returned zero.

The only wrinkle I can see is the bug called out in the commit that added this
code (the wonderfully (extreme sarcasm) titled commit 24647e0a39b6, "KVM: x86:
Return updated timer current count register from KVM_GET_LAPIC")

 : Note: When a one-shot timer expires, the code in arch/x86/kvm/lapic.c does
 : not zero the value of the LAPIC initial count register (emulating HW
 : behavior). If no other timer is run and pending prior to a subsequent
 : KVM_GET_LAPIC call, the returned register set will include the expired
 : one-shot initial count. On a subsequent KVM_SET_LAPIC call the code will
 : see a non-zero initial count and start a new one-shot timer using the
 : expired timer's count. This is a prior existing bug and will be addressed
 : in a separate patch. Thanks to jmattson@google.com for this find.

I don't see any evidence that that bug was ever fixed.  But again, that's an
orthogonal bug.

There is commit 2735886c9ef1 ("KVM: LAPIC: Keep stored TMCCT register value 0
after KVM_SET_LAPIC"), but I'm struggling to see how that's anything but a glorified
nop.

>  			else if (unlikely(deadline > apic->lapic_timer.period)) {
>  				pr_info_ratelimited(
>  				    "kvm: vcpu %i: requested lapic timer restore with "
> -- 
> 2.9.4
>