| Message ID | 20170502213616.GA24837@amt.cnet (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 02/05/2017 23:36, Marcelo Tosatti wrote: > In the masterclock enabled case, kvmclock_offset must be adjusted so > that user_ns.clock = master_kernel_ns + kvmclock_offset (that is, the > value set from KVM_SET_CLOCK is the one visible at system_timestamp). > > This way the guest clock: > > 1. Starts counting when KVM_SET_CLOCK executes. > 2. With the value provided by userspace. So this fixes rounding errors? > - now_ns = get_kvmclock_ns(kvm); > - kvm->arch.kvmclock_offset += user_ns.clock - now_ns; > + > kvm_gen_update_masterclock(kvm); > + if (ka->use_master_clock) { > + /* > + * In the masterclock enabled case, > + * kvmclock_offset must be adjusted so that > + * user_ns.clock = master_kernel_ns + kvmclock_offset > + * (that is, the value set from KVM_SET_CLOCK is the > + * one visible at system_timestamp). > + */ > + kvm->arch.kvmclock_offset = user_ns.clock - > + ka->master_kernel_ns; > + This needs to hold ka->pvclock_gtod_sync_lock, I think. > + kvm_for_each_vcpu(i, vcpu, kvm) > + kvm_make_request(KVM_REQ_CLOCK_UPDATE, vcpu); kvm_gen_update_masterclock already does that, why did you move that to before the assignment? Paolo
On Wed, May 03, 2017 at 03:08:46PM +0200, Paolo Bonzini wrote: > > > On 02/05/2017 23:36, Marcelo Tosatti wrote: > > In the masterclock enabled case, kvmclock_offset must be adjusted so > > that user_ns.clock = master_kernel_ns + kvmclock_offset (that is, the > > value set from KVM_SET_CLOCK is the one visible at system_timestamp). > > > > This way the guest clock: > > > > 1. Starts counting when KVM_SET_CLOCK executes. > > 2. With the value provided by userspace. > > So this fixes rounding errors? No. It just does the correct "user_ns.clock = master_kernel_ns + kvmclock_offset = return value of get_kvmclock_ns(kvm) at moment of KVM_SET_CLOCK" (that is, when guest_rdtsc() == tsc_timestamp, so guest_rdtsc() - tsc_timestamp = 0, you want get_kvmclock_ns() to return user_ns.clock). Its not a rounding error. > > - now_ns = get_kvmclock_ns(kvm); > > - kvm->arch.kvmclock_offset += user_ns.clock - now_ns; > > + > > kvm_gen_update_masterclock(kvm); > > + if (ka->use_master_clock) { > > + /* > > + * In the masterclock enabled case, > > + * kvmclock_offset must be adjusted so that > > + * user_ns.clock = master_kernel_ns + kvmclock_offset > > + * (that is, the value set from KVM_SET_CLOCK is the > > + * one visible at system_timestamp). > > + */ > > + kvm->arch.kvmclock_offset = user_ns.clock - > > + ka->master_kernel_ns; > > + > > > This needs to hold ka->pvclock_gtod_sync_lock, I think. > > > + kvm_for_each_vcpu(i, vcpu, kvm) > > + kvm_make_request(KVM_REQ_CLOCK_UPDATE, vcpu); > > kvm_gen_update_masterclock already does that, why did you move that to > before the assignment? Its after the assignment of kvmclock_offset because KVM_REQ_CLOCK_UPDATES processing by vcpus make use of kvmclock_offset. So if you change that value, you have to request the vcpus to recalculate their kvmclock areas using the new kvmclock_offset. Resending v2, thanks.
On 03/05/2017 15:40, Marcelo Tosatti wrote: >>> In the masterclock enabled case, kvmclock_offset must be adjusted so >>> that user_ns.clock = master_kernel_ns + kvmclock_offset (that is, the >>> value set from KVM_SET_CLOCK is the one visible at system_timestamp). >>> >>> This way the guest clock: >>> >>> 1. Starts counting when KVM_SET_CLOCK executes. >>> 2. With the value provided by userspace. >> >> So this fixes rounding errors? > > No. It just does the correct > > "user_ns.clock = master_kernel_ns + kvmclock_offset = return value of > get_kvmclock_ns(kvm) at moment of KVM_SET_CLOCK" > > (that is, when guest_rdtsc() == tsc_timestamp, so guest_rdtsc() - tsc_timestamp = 0, > you want get_kvmclock_ns() to return user_ns.clock). Yep, and it currently doesn't match because the current code introduces rounding errors? >>> + kvm_for_each_vcpu(i, vcpu, kvm) >>> + kvm_make_request(KVM_REQ_CLOCK_UPDATE, vcpu); >> kvm_gen_update_masterclock already does that, why did you move that to >> before the assignment? > Its after the assignment of kvmclock_offset because > KVM_REQ_CLOCK_UPDATES processing by vcpus make use of kvmclock_offset. Sure, but why did you move kvm_gen_update_masterclock before? If you left kvm_gen_update_masterclock after the update of kvm->arch.kvmclock_offset, the duplicate KVM_REQ_CLOCK_UPDATE would not be necessary. Paolo > So if you change that value, you have to request the vcpus to > recalculate their kvmclock areas using the new kvmclock_offset.
On Wed, May 03, 2017 at 03:55:36PM +0200, Paolo Bonzini wrote: > > > On 03/05/2017 15:40, Marcelo Tosatti wrote: > >>> In the masterclock enabled case, kvmclock_offset must be adjusted so > >>> that user_ns.clock = master_kernel_ns + kvmclock_offset (that is, the > >>> value set from KVM_SET_CLOCK is the one visible at system_timestamp). > >>> > >>> This way the guest clock: > >>> > >>> 1. Starts counting when KVM_SET_CLOCK executes. > >>> 2. With the value provided by userspace. > >> > >> So this fixes rounding errors? > > > > No. It just does the correct > > > > "user_ns.clock = master_kernel_ns + kvmclock_offset = return value of > > get_kvmclock_ns(kvm) at moment of KVM_SET_CLOCK" > > > > (that is, when guest_rdtsc() == tsc_timestamp, so guest_rdtsc() - tsc_timestamp = 0, > > you want get_kvmclock_ns() to return user_ns.clock). > > Yep, and it currently doesn't match because the current code introduces > rounding errors? No. Consider the current code. When get_kvmclock_ns() is called, ka->masterclock = false. kvm_gen_update_masterclock() sets it to true (because at the time KVM_SET_CLOCK is called, the conditions necessary to do so are fulfilled). > >>> + kvm_for_each_vcpu(i, vcpu, kvm) > >>> + kvm_make_request(KVM_REQ_CLOCK_UPDATE, vcpu); > >> kvm_gen_update_masterclock already does that, why did you move that to > >> before the assignment? > > Its after the assignment of kvmclock_offset because > > KVM_REQ_CLOCK_UPDATES processing by vcpus make use of kvmclock_offset. > > Sure, but why did you move kvm_gen_update_masterclock before? Explained above. > If you left kvm_gen_update_masterclock after the update of > kvm->arch.kvmclock_offset, the duplicate KVM_REQ_CLOCK_UPDATE would not > be necessary. No can do. Feel free to reorganize/rewrite the patch if you feel like it, as long as the same effects are achieved. Thanks. > > Paolo > > > So if you change that value, you have to request the vcpus to > > recalculate their kvmclock areas using the new kvmclock_offset.
Index: kvm/arch/x86/kvm/x86.c =================================================================== --- kvm.orig/arch/x86/kvm/x86.c 2017-04-27 17:37:48.131348255 -0300 +++ kvm/arch/x86/kvm/x86.c 2017-04-27 17:56:58.397530444 -0300 @@ -4172,8 +4172,9 @@ break; } case KVM_SET_CLOCK: { - struct kvm_clock_data user_ns; u64 now_ns; + struct kvm_clock_data user_ns; + struct kvm_arch *ka = &kvm->arch; r = -EFAULT; if (copy_from_user(&user_ns, argp, sizeof(user_ns))) @@ -4184,9 +4185,25 @@ goto out; r = 0; - now_ns = get_kvmclock_ns(kvm); - kvm->arch.kvmclock_offset += user_ns.clock - now_ns; + kvm_gen_update_masterclock(kvm); + if (ka->use_master_clock) { + /* + * In the masterclock enabled case, + * kvmclock_offset must be adjusted so that + * user_ns.clock = master_kernel_ns + kvmclock_offset + * (that is, the value set from KVM_SET_CLOCK is the + * one visible at system_timestamp). + */ + kvm->arch.kvmclock_offset = user_ns.clock - + ka->master_kernel_ns; + + kvm_for_each_vcpu(i, vcpu, kvm) + kvm_make_request(KVM_REQ_CLOCK_UPDATE, vcpu); + } else { + now_ns = get_kvmclock_ns(kvm); + kvm->arch.kvmclock_offset += user_ns.clock - now_ns; + } break; } case KVM_GET_CLOCK: {
In the masterclock enabled case, kvmclock_offset must be adjusted so that user_ns.clock = master_kernel_ns + kvmclock_offset (that is, the value set from KVM_SET_CLOCK is the one visible at system_timestamp). This way the guest clock: 1. Starts counting when KVM_SET_CLOCK executes. 2. With the value provided by userspace. Signed-off-by: Marcelo Tosatti <mtosatti@redhat.com> --- arch/x86/kvm/x86.c | 23 ++++++++++++++++++++--- 1 file changed, 20 insertions(+), 3 deletions(-)