On Tue, 2023-10-31 at 12:22 +0000, Paul Durrant wrote: > > > > > As I said, this patch stands even *after* we fix kvmclock, because > > it handles the timer delta calculation from an single TSC read. > > > > But overengineering a timer reset on KVM_REQ_CLOCK_UPDATE would not. > > I'm not sure what you intend to do to kvmlock, so not sure whether we'll > still need the __pvclock_read_cycles(&vcpu->arch.hv_clock, guest_tsc) > but this patch (with the extra check on validity of hv_clock) does fix > the drift so... > > Reviewed-by: Paul Durrant <paul@xxxxxxx> Ta. And no, I'm not quite sure what I'm going to do with kvmclock for the general case yet. The more I look at it, the more I realise how broken it is. Last week I thought it was just about the way KVM 'jumps' the kvmclock and yanks it back to the host's CLOCK_MONOTONIC_RAW, but I thought KVM at *least* managed to do it right between those times. But no, this patch is addressing the fact that even *without* those clock jumps, KVM doesn't manage to calculate the guest clock the same way that it tells the guest to... and thus doesn't get the same results :) I think it involves get_kvmclock_ns() using the frequency given in the KVM-wide (not per-vCPU) KVM_SET_TSC_KHZ ioctl, and scaling via that to get the guest clock. That should match, without having to have a specific vCPU's hv_clock to play with. And maybe we can also have a get_kvmclock_ns_at() which takes a host TSC value, and the timer code from this patch can use that instead of using __pvclock_read_cycles() directly. That's probably the easy part. Fixing the 'resync' to CLOCK_MONOTONIC_RAW, while keeping things working for the now- considered-pathological !CONSTANT_TSC case, will be slightly more fun. As well as suspend etc. in the CONSTANT_TSC case, of course. And replacing that stupid KVM_CLOCK_REALTIME with something that uses CLOCK_TAI. Or maybe just making it export the tai_offset at the same moment?
Attachment:
smime.p7s
Description: S/MIME cryptographic signature