Re: [PATCH v5 15/34] KVM: x86: Restructure kvm_guest_time_update() for TSC upscaling

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] Dropping the `local_irq_save()` protection around the TSC catchup calculation exposes an unprotected Read-Modify-Write (RMW) of `l1_tsc_offset`, leading to guest TSC corruption on systems with unstable TSC.
- [Medium] The commit message claims a WARN is added for `kvm_get_time_and_clockread()` unexpected failures, but the implementation silently handles it.
--

commit f05935e83e34b920673a8039b5e1b6c7443ffe07
Author: David Woodhouse <dwmw@amazon.co.uk>

Restructure kvm_guest_time_update() so that kernel_ns/host_tsc are
always "now" when doing TSC catchup, then swap in the master clock
reference values afterward for the hv_clock.

This makes the TSC upscaling code considerably simpler: the catchup
adjustment is computed as the delta between what the guest TSC *should*
be at "now" and what it actually is, rather than mixing "now" and
"master clock reference" timestamps.

> The seqcount loop now also contains the kvm_get_time_and_clockread()
> call (matching get_kvmclock's pattern), with the same WARN for
> unexpected failure.

[Severity: Medium]
The commit message mentions that a WARN is added for unexpected failures of
kvm_get_time_and_clockread(), but the implementation inside the
CONFIG_X86_64 block seems to silently handle the failure by clearing
use_master_clock. Was the WARN intentionally omitted from the patch?

> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> index 8aae22401046..92e32d720523 100644
> --- a/arch/x86/kvm/x86.c
> +++ b/arch/x86/kvm/x86.c
[ ... ]
> @@ -3363,46 +3363,63 @@ static void kvm_setup_guest_pvclock(struct pvclock_vcpu_time_info *ref_hv_clock,
[ ... ]
> -	/* Keep irq disabled to prevent changes to the clock */
> -	local_irq_save(flags);
> -	tgt_tsc_hz = (u64)get_cpu_tsc_khz() * 1000;
>  	if (unlikely(tgt_tsc_hz == 0)) {
> -		local_irq_restore(flags);
>  		kvm_make_request(KVM_REQ_CLOCK_UPDATE, v);
>  		return 1;
>  	}
> +
>  	if (!use_master_clock) {
> +		unsigned long flags;
> +
> +		local_irq_save(flags);
>  		host_tsc = rdtsc();
>  		kernel_ns = get_kvmclock_base_ns();
> +		local_irq_restore(flags);
>  	}
>  
> -	tsc_timestamp = kvm_read_l1_tsc(v, host_tsc);
> -
>  	/*
>  	 * We may have to catch up the TSC to match elapsed wall clock
>  	 * time for two reasons, even if kvmclock is used.
> @@ -3411,17 +3428,32 @@ int kvm_guest_time_update(struct kvm_vcpu *v)
>  	 *      entry to avoid unknown leaps of TSC even when running
>  	 *      again on the same CPU.  This may cause apparent elapsed
>  	 *      time to disappear, and the guest to stand still or run
> -	 *	very slowly.
> +	 *      very slowly.
>  	 */
>  	if (vcpu->tsc_catchup) {
> -		u64 tsc = compute_guest_tsc(v, kernel_ns);
> -		if (tsc > tsc_timestamp) {
> -			adjust_tsc_offset_guest(v, tsc - tsc_timestamp);
> -			tsc_timestamp = tsc;
> -		}
> +		s64 adjustment;
> +
> +		/*
> +		 * Calculate the delta between what the guest TSC *should* be
> +		 * and what it actually is according to kvm_read_l1_tsc().
> +		 */
> +		adjustment = compute_guest_tsc(v, kernel_ns) -
> +			     kvm_read_l1_tsc(v, host_tsc);
> +		if (adjustment > 0)
> +			adjust_tsc_offset_guest(v, adjustment);

[Severity: High]
Does dropping the local_irq_save() protection around the TSC catchup calculation
expose an unprotected read-modify-write of l1_tsc_offset?

If preemption occurs inside adjust_tsc_offset_guest() after reading the offset:

arch/x86/kvm/x86.c:adjust_tsc_offset_guest() {
    u64 tsc_offset = vcpu->arch.l1_tsc_offset;
    /* <--- preemption here */
    kvm_vcpu_write_tsc_offset(vcpu, tsc_offset + adjustment);
}

And the vCPU migrates to a new CPU with an unstable TSC, KVM's preempt
notifiers would call kvm_arch_vcpu_load() and write a new compensation
offset. When this thread resumes, wouldn't it write back the stale base
offset and overwrite the preempt notifier's compensation?

Since the code only corrects positive adjustments, the corrupted offset
would remain, potentially causing the guest TSC to leap forward.

>  	}
>  
> -	local_irq_restore(flags);

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260608145455.89187-1-dwmw2@infradead.org?part=15