On Tue, 2026-08-11 at 16:40 -0700, Sean Christopherson wrote: > > Invoking kvm_guest_time_update() here is probably a deal-breaker. Updating > the > master clock and other internal state is far from ideal, but should be ok. > > However, writing guest memory is not. Specifically, dirtying memory after the > last KVM_RUN is a non-starter for many usecases, as is modifying state that is > visible via other GET uAPI (though I don't think that applies here?). E.g. > see > commits:
Ack. Calling kvm_guest_time_update() is also *entirely* pointless. We are in masterclock mode, by definition, as this ioctl only works in masterclock mode. So kvm_guest_time_update() isn't actually creating any new information; it just calculates the same per-VM tsc_shift/mul. And kvm_vcpu_ioctl_get_clock_guest() could just use those directly. Even better, it can do so *inside* the seqcount loop. Fixup below (not amending commits while you're co-opting the tree, as we'd both go insane). I'll push it to the top of https://git.infradead.org/?p=users/dwmw2/linux.git;a=shortlog;h=refs/heads/kvmclock9-part2 on top of with the offset test which is already there. > And we'd probably want to build on my idea to report that > KVM_RUN needs completion[*], but that'd be a good thing overall. > > https://lore.kernel.org/all/[email protected] Aha... *that* is why I found that lore thread open in my browser yesterday; I was confused about how I'd got there. It's missing KVM_EXIT_XEN btw. (Of which, KVM_EXIT_XEN_HYPERCALL is the only subtype. I suspect João originally expected that there would be more). From d2e54f439f0a5c0f2bddeaead916b23daedb2b2f Mon Sep 17 00:00:00 2001 From: David Woodhouse <[email protected]> Date: Fri, 14 Aug 2026 07:56:58 +0100 Subject: [PATCH] fixup! KVM: x86: Add KVM_[GS]ET_CLOCK_GUEST for accurate KVM clock migration Make KVM_GET_CLOCK_GUEST stateless, never writing guest memory. Invoking kvm_guest_time_update() from the GET ioctl writes the guest's pvclock pages, and dirtying guest memory after the final KVM_RUN breaks migration flows which have already completed their last dirty-log pass. There is no need for it. The only outputs previously taken from the vCPU's shadow pvclock were tsc_shift and tsc_to_system_mul, and in master clock mode (which this ioctl requires; it returns -ENODATA otherwise) those are identical to the master clock's own mul/shift: both are computed by kvm_get_time_scale() from the same guest TSC frequency, which all vCPUs share. Use ka->master_tsc_{shift,mul} directly, which pvclock_update_vm_gtod_copy() precomputed for exactly this kind of vCPU-less consumer. The only remaining per-vCPU input is kvm_read_l1_tsc(), which is a pure read, translating the master clock snapshot into the vCPU's own TSC domain. This also removes the -EBUSY case and the special handling of a vCPU which has never run: the master clock state exists from VM creation, so the pvclock can always be constructed. And it moves the reads of tsc_shift, tsc_to_system_mul and flags inside the seqcount loop, where they should have been all along: previously they could tear against a concurrent master clock update. A KVM_REQ_CLOCK_UPDATE pending on the vCPU is now left pending rather than being consumed: the pvclock returned here is anchored to the current master clock snapshot and describes the same linear function of the guest TSC as the guest-visible copy, so there is nothing to refresh. The guest's own pages are updated, as ever, only from KVM_RUN. Signed-off-by: David Woodhouse <[email protected]> --- arch/x86/kvm/x86.c | 37 ++++++++++++++++--------------------- 1 file changed, 16 insertions(+), 21 deletions(-) diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c index 8eb73e3ada66..4de98cc16751 100644 --- a/arch/x86/kvm/x86.c +++ b/arch/x86/kvm/x86.c @@ -3460,27 +3460,22 @@ static int kvm_vcpu_ioctl_enable_cap(struct kvm_vcpu *vcpu, static int kvm_vcpu_ioctl_get_clock_guest(struct kvm_vcpu *v, void __user *argp) { struct pvclock_vcpu_time_info hv_clock = {}; - struct kvm_vcpu_arch *vcpu = &v->arch; struct kvm_arch *ka = &v->kvm->arch; unsigned int seq; /* - * If KVM_REQ_CLOCK_UPDATE is already pending, or if the pvclock - * has never been generated at all, call kvm_guest_time_update(). - * Its only failure mode is transient (the TSC frequency of the - * current CPU is momentarily unknown), so return -EBUSY to tell - * userspace to try again. - */ - if (kvm_check_request(KVM_REQ_CLOCK_UPDATE, v) || !vcpu->hw_tsc_hz) { - guard(srcu)(&v->kvm->srcu); - - if (kvm_guest_time_update(v)) - return -EBUSY; - } - - /* - * Reconstruct the pvclock from the master clock state, matching - * exactly what kvm_guest_time_update() writes to the guest. + * Construct the pvclock purely from the master clock state. The + * master mul/shift are computed for the guest TSC frequency, which + * in master clock mode is the frequency of every vCPU; only the + * tsc_timestamp is per-vCPU, translating the master snapshot into + * this vCPU's TSC domain via its scaling ratio and offset. + * + * Note, this deliberately does NOT invoke kvm_guest_time_update(), + * which would write the guest's pvclock pages: dirtying guest + * memory after the final KVM_RUN would break post-copy migration + * flows. The pvclock returned here describes the same clock as the + * guest-visible copy (the same linear function of the guest TSC), + * anchored at the current master clock snapshot. */ do { seq = read_seqcount_begin(&ka->pvclock_sc); @@ -3490,12 +3485,12 @@ static int kvm_vcpu_ioctl_get_clock_guest(struct kvm_vcpu *v, void __user *argp) hv_clock.tsc_timestamp = kvm_read_l1_tsc(v, ka->master_cycle_now); hv_clock.system_time = ka->master_kernel_ns + ka->kvmclock_offset; + hv_clock.tsc_shift = ka->master_tsc_shift; + hv_clock.tsc_to_system_mul = ka->master_tsc_mul; + hv_clock.flags = ka->all_vcpus_matched_tsc ? + PVCLOCK_TSC_STABLE_BIT : 0; } while (read_seqcount_retry(&ka->pvclock_sc, seq)); - hv_clock.tsc_shift = vcpu->pvclock_tsc_shift; - hv_clock.tsc_to_system_mul = vcpu->pvclock_tsc_mul; - hv_clock.flags = ka->all_vcpus_matched_tsc ? PVCLOCK_TSC_STABLE_BIT : 0; - if (copy_to_user(argp, &hv_clock, sizeof(hv_clock))) return -EFAULT; -- 2.43.0
smime.p7s
Description: S/MIME cryptographic signature
