Re: [PATCH v7 28/36] KVM: x86: Activate master clock from kvm_arch_init_vm()

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] Incorrect masterclock scaling ratio and stale state initialization in `kvm_arch_init_vm()`.
- [High] Unconditional setting of `user_set_tsc = true` at vCPU creation breaks 1-second TSC slop protection.
--

--- Patch [28]: [PATCH v7 28/36] KVM: x86: Activate master clock from kvm_arch_init_vm() ---
commit c68bcfc5acfe3bb9dda51b0de8e297101615b7d5
Author: David Woodhouse <dwmw@amazon.co.uk>

KVM: x86: Activate master clock from kvm_arch_init_vm()

Initialize the TSC generation and master clock state at VM creation
time, rather than relying on kvm_update_masterclock() from
kvm_arch_vcpu_postcreate().

> --- a/arch/x86/kvm/x86.c
> +++ b/arch/x86/kvm/x86.c
> @@ -9407,8 +9407,6 @@ void kvm_arch_vcpu_postcreate(struct kvm_vcpu *vcpu)
>  		return;
>  	vcpu_load(vcpu);
>  	kvm_synchronize_tsc(vcpu, NULL);

[Severity: High]
A previous patch in this series modified kvm_set_tsc_khz() to unconditionally
call kvm_synchronize_tsc(vcpu, &tsc) with a non-NULL pointer during vCPU
initialization.

Does this cause __kvm_synchronize_tsc() to unconditionally set
kvm->arch.user_set_tsc = true globally for the VM when the very first vCPU
is created?

If user_set_tsc is set immediately at boot, does this break the 1-second TSC
slop protection for newly restored VMs, potentially leading to timekeeping
corruption and TSC jumps during live migration of young VMs?

> -	if (!vcpu->kvm->arch.use_master_clock)
> -		kvm_update_masterclock(vcpu->kvm, NULL);

[Severity: High]
By removing this synchronous update, are we deferring the
master_tsc_scaling_ratio updates to KVM_RUN? 

If so, does this leave early KVM_GET_CLOCK requests with a stale ratio?

>  	vcpu_put(vcpu);

[ ... ]

> @@ -9849,18 +9849,47 @@ int kvm_arch_init_vm(struct kvm *kvm, unsigned long type)
[ ... ]
>  	kvm->arch.default_tsc_khz = max_tsc_khz ? : tsc_khz;
>  	kvm->arch.apic_bus_cycle_ns = APIC_BUS_CYCLE_NS_DEFAULT;
>  	kvm->arch.guest_can_read_msr_platform_info = true;
>  	kvm->arch.enable_pmu = enable_pmu && !kvm->arch.has_protected_pmu;
>  
> +	kvm->arch.all_vcpus_matched_freq = true;
> +	kvm->arch.all_vcpus_matched_tsc = true;
> +	kvm->arch.cur_tsc_scaling_ratio = kvm_caps.default_tsc_scaling_ratio;
> +	kvm->arch.cur_tsc_khz = kvm->arch.default_tsc_khz;

[Severity: High]
Here we initialize cur_tsc_khz to default_tsc_khz, which might be scaled
if max_tsc_khz is configured on the host.

> +
> +	raw_spin_lock_irqsave(&kvm->arch.tsc_write_lock, flags);
[ ... ]
> +	kvm->arch.cur_tsc_write = 0;
> +	kvm->arch.cur_tsc_offset = -(s64)kvmclock_host_tsc;

[Severity: High]
However, cur_tsc_offset is set to the unscaled -(s64)kvmclock_host_tsc here.

When the first vCPU is created, kvm_synchronize_tsc() sees
virtual_tsc_khz == cur_tsc_khz. Will it then erroneously apply this unscaled
offset to a vCPU using a scaled multiplier, since we bypass the scaling that
kvm_compute_l1_tsc_offset() would have provided?

Could this lead to a large guest TSC jump at boot and cause timekeeping
corruption?

> +	kvm->arch.cur_tsc_nsec = kvmclock_ns;

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