Re: [PATCH v6 2/2] KVM: VMX: Cap VMX preemption timer to work around Intel erratum

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] The computed VMX preemption timer maximum value can exceed 32 bits on modern CPUs because it is not clamped to the architectural limit (UINT_MAX). This leads to silent 32-bit truncation when written to the VMCS, breaking the timer's soft-disable mechanism and causing premature guest timer expirations.
--

--- Patch [2]: [PATCH v6 2/2] KVM: VMX: Cap VMX preemption timer to work around Intel erratum ---
commit 021d423821c3a8ec582691801fee4d57b9a316fc
Author: Jim Mattson <jmattson@google.com>

KVM: VMX: Cap VMX preemption timer to work around Intel erratum

This commit introduces a workaround for an Intel erratum where the VMX
preemption timer can expire early if programmed with large values. It
calculates a safe maximum value based on CPUID.15H. This maximum is then
used when the timer is soft-disabled and to bound delta_tsc.

Closes: https://lore.kernel.org/all/Zn9X0yFxZi_Mrlnt@google.com/

> diff --git a/arch/x86/kvm/vmx/vmx.c b/arch/x86/kvm/vmx/vmx.c
> index 7d2cc36a04e51..67c8dc1eb2c4f 100644
> --- a/arch/x86/kvm/vmx/vmx.c
> +++ b/arch/x86/kvm/vmx/vmx.c

[ ... ]

> @@ -8305,6 +8306,33 @@ static inline int u64_shl_div_u64(u64 a, unsigned int shift,
>  	return 0;
>  }
>  
> +/*
> + * Workaround for a widespread Intel erratum (e.g. EMR158) where the
> + * VMX-preemption timer may expire earlier than expected when programmed
> + * with large values. The workaround is to cap the timer value to strictly
> + * less than 2^25 * CPUID.15H:EBX / CPUID.15H:EAX.
> + */
> +static __init u64 calc_preemption_timer_max_value(void)
> +{
> +	const u64 ARCHITECTURAL_MAX_VALUE = UINT_MAX;
> +	u32 eax, ebx, ecx, edx;

[ ... ]

> +	if (WARN_ON_ONCE(!(((u64)ebx << 25) / eax)))
> +		return ARCHITECTURAL_MAX_VALUE;
> +
> +	return (((u64)ebx << 25) / eax) - 1;

[Severity: High]
If a CPU has a high TSC-to-crystal clock ratio (for example, EBX/EAX >= 128),
could this calculation exceed UINT_MAX? Because the returned u64 is not clamped
to ARCHITECTURAL_MAX_VALUE, preemption_timer_max_value could end up storing a
value larger than 32 bits.

> +}

[ ... ]

> @@ -8362,12 +8392,12 @@ int vmx_set_hv_timer(struct kvm_vcpu *vcpu, u64 guest_deadline_tsc,
>  		return -ERANGE;
>  
>  	/*
> -	 * If the delta tsc can't fit in the 32 bit after the multi shift,
> -	 * we can't use the preemption timer.
> +	 * If the delta tsc exceeds the preemption timer limit after the
> +	 * multi shift, we can't use the preemption timer.
>  	 * It's possible that it fits on later vmentries, but checking
>  	 * on every vmentry is costly so we just use an hrtimer.
>  	 */
> -	if (delta_tsc >> (cpu_preemption_timer_multi + 32))
> +	if ((delta_tsc >> cpu_preemption_timer_multi) > preemption_timer_max_value)
>  		return -ERANGE;

[Severity: High]
If preemption_timer_max_value is greater than UINT_MAX, does this condition fail
to catch delta_tsc limits that exceed 32 bits? It appears this might bypass the
required hrtimer fallback for large guest deadlines, as the value will later be
silently truncated.

>  
>  	vmx->hv_deadline_tsc = tscl + delta_tsc;

[ ... ]

> @@ -8401,7 +8431,7 @@ static void vmx_update_hv_timer(struct kvm_vcpu *vcpu, bool force_immediate_exit
>  		vmcs_write32(VMX_PREEMPTION_TIMER_VALUE, delta_tsc);
>  		vmx->loaded_vmcs->hv_timer_soft_disabled = false;
>  	} else if (!vmx->loaded_vmcs->hv_timer_soft_disabled) {
> -		vmcs_write32(VMX_PREEMPTION_TIMER_VALUE, -1);
> +		vmcs_write32(VMX_PREEMPTION_TIMER_VALUE, preemption_timer_max_value);
>  		vmx->loaded_vmcs->hv_timer_soft_disabled = true;
>  	}
>  }

[Severity: High]
Will writing an un-clamped u64 to vmcs_write32() cause it to be truncated? If
truncated, the preemption timer could fire prematurely when soft-disabled
instead of waiting for the maximum duration.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260729134203.1377606-1-seanjc@google.com?part=2