[PATCH 0/2] KVM: x86: abstract locking around pvclock_update_vm_gtod_copy

[PATCH 0/2] KVM: x86: abstract locking around pvclock_update_vm_gtod_copy

[Paolo Bonzini]

Two cleanup patches that factor out the handling of
KVM_REQ_MASTERCLOCK_UPDATE and KVM_REQ_MCLOCK_INPROGRESS.  I have another
patch actually to remove KVM_REQ_MCLOCK_INPROGRESS, but I don't have time
to finish testing it right now; so here are only the cleanups leading
to it.

Paolo Bonzini (2):
  KVM: KVM-on-hyperv: shorten no-entry section on reenlightenment
  kvm: x86: abstract locking around pvclock_update_vm_gtod_copy

 arch/x86/include/asm/kvm_host.h |  1 -
 arch/x86/kvm/x86.c              | 67 +++++++++++++--------------------
 2 files changed, 27 insertions(+), 41 deletions(-)

-- 
2.27.0

[PATCH 1/2] KVM: KVM-on-hyperv: shorten no-entry section on reenlightenment

[Paolo Bonzini]

During re-enlightenment, update kvmclock a VM at a time instead of
raising KVM_REQ_MASTERCLOCK_UPDATE for all VMs.  Because the guests
can now run after TSC emulation has been disabled, invalidate
their TSC page so that they refer to the reference time counter
MSR while the update is in progress.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 arch/x86/kvm/x86.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index bab8eb3e0a47..284afaa1db86 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -8111,7 +8111,7 @@ static void kvm_hyperv_tsc_notifier(void)
 
 	mutex_lock(&kvm_lock);
 	list_for_each_entry(kvm, &vm_list, vm_list)
-		kvm_make_mclock_inprogress_request(kvm);
+		kvm_hv_invalidate_tsc_page(kvm);
 
 	hyperv_stop_tsc_emulation();
 
@@ -8123,6 +8123,7 @@ static void kvm_hyperv_tsc_notifier(void)
 	list_for_each_entry(kvm, &vm_list, vm_list) {
 		struct kvm_arch *ka = &kvm->arch;
 
+		kvm_make_mclock_inprogress_request(kvm);
 		spin_lock_irqsave(&ka->pvclock_gtod_sync_lock, flags);
 		pvclock_update_vm_gtod_copy(kvm);
 		spin_unlock_irqrestore(&ka->pvclock_gtod_sync_lock, flags);
-- 
2.27.0

[PATCH 2/2] kvm: x86: abstract locking around pvclock_update_vm_gtod_copy

[Paolo Bonzini]

Updates to the kvmclock parameters needs to do a complicated dance of
KVM_REQ_MCLOCK_INPROGRESS and KVM_REQ_CLOCK_UPDATE in addition to taking
pvclock_gtod_sync_lock.  Place that in two functions that can be called
on all of master clock update, KVM_SET_CLOCK, and Hyper-V reenlightenment.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 arch/x86/include/asm/kvm_host.h |  1 -
 arch/x86/kvm/x86.c              | 66 +++++++++++++--------------------
 2 files changed, 26 insertions(+), 41 deletions(-)

diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h
index bc6157677753..c54f9da91f99 100644
--- a/arch/x86/include/asm/kvm_host.h
+++ b/arch/x86/include/asm/kvm_host.h
@@ -1839,7 +1839,6 @@ u64 kvm_calc_nested_tsc_multiplier(u64 l1_multiplier, u64 l2_multiplier);
 unsigned long kvm_get_linear_rip(struct kvm_vcpu *vcpu);
 bool kvm_is_linear_rip(struct kvm_vcpu *vcpu, unsigned long linear_rip);
 
-void kvm_make_mclock_inprogress_request(struct kvm *kvm);
 void kvm_make_scan_ioapic_request(struct kvm *kvm);
 void kvm_make_scan_ioapic_request_mask(struct kvm *kvm,
 				       unsigned long *vcpu_bitmap);
diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index 284afaa1db86..8f96cdfcf364 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -2757,35 +2757,36 @@ static void pvclock_update_vm_gtod_copy(struct kvm *kvm)
 #endif
 }
 
-void kvm_make_mclock_inprogress_request(struct kvm *kvm)
+static void kvm_start_pvclock_update(struct kvm *kvm)
 {
+	struct kvm_arch *ka = &kvm->arch;
 	kvm_make_all_cpus_request(kvm, KVM_REQ_MCLOCK_INPROGRESS);
+
+	/* no guest entries from this point */
+	spin_lock_irq(&ka->pvclock_gtod_sync_lock);
 }
 
-static void kvm_gen_update_masterclock(struct kvm *kvm)
+static void kvm_end_pvclock_update(struct kvm *kvm)
 {
-#ifdef CONFIG_X86_64
-	int i;
-	struct kvm_vcpu *vcpu;
 	struct kvm_arch *ka = &kvm->arch;
-	unsigned long flags;
-
-	kvm_hv_invalidate_tsc_page(kvm);
-
-	kvm_make_mclock_inprogress_request(kvm);
-
-	/* no guest entries from this point */
-	spin_lock_irqsave(&ka->pvclock_gtod_sync_lock, flags);
-	pvclock_update_vm_gtod_copy(kvm);
-	spin_unlock_irqrestore(&ka->pvclock_gtod_sync_lock, flags);
+	struct kvm_vcpu *vcpu;
+	int i;
 
+	spin_unlock_irq(&ka->pvclock_gtod_sync_lock);
 	kvm_for_each_vcpu(i, vcpu, kvm)
 		kvm_make_request(KVM_REQ_CLOCK_UPDATE, vcpu);
 
 	/* guest entries allowed */
 	kvm_for_each_vcpu(i, vcpu, kvm)
 		kvm_clear_request(KVM_REQ_MCLOCK_INPROGRESS, vcpu);
-#endif
+}
+
+static void kvm_update_masterclock(struct kvm *kvm)
+{
+	kvm_hv_invalidate_tsc_page(kvm);
+	kvm_start_pvclock_update(kvm);
+	pvclock_update_vm_gtod_copy(kvm);
+	kvm_end_pvclock_update(kvm);
 }
 
 u64 get_kvmclock_ns(struct kvm *kvm)
@@ -6077,12 +6078,10 @@ long kvm_arch_vm_ioctl(struct file *filp,
 			goto out;
 
 		r = 0;
-		/*
-		 * TODO: userspace has to take care of races with VCPU_RUN, so
-		 * kvm_gen_update_masterclock() can be cut down to locked
-		 * pvclock_update_vm_gtod_copy().
-		 */
-		kvm_gen_update_masterclock(kvm);
+
+		kvm_hv_invalidate_tsc_page(kvm);
+		kvm_start_pvclock_update(kvm);
+		pvclock_update_vm_gtod_copy(kvm);
 
 		/*
 		 * This pairs with kvm_guest_time_update(): when masterclock is
@@ -6091,15 +6090,12 @@ long kvm_arch_vm_ioctl(struct file *filp,
 		 * is slightly ahead) here we risk going negative on unsigned
 		 * 'system_time' when 'user_ns.clock' is very small.
 		 */
-		spin_lock_irq(&ka->pvclock_gtod_sync_lock);
 		if (kvm->arch.use_master_clock)
 			now_ns = ka->master_kernel_ns;
 		else
 			now_ns = get_kvmclock_base_ns();
 		ka->kvmclock_offset = user_ns.clock - now_ns;
-		spin_unlock_irq(&ka->pvclock_gtod_sync_lock);
-
-		kvm_make_all_cpus_request(kvm, KVM_REQ_CLOCK_UPDATE);
+		kvm_end_pvclock_update(kvm);
 		break;
 	}
 	case KVM_GET_CLOCK: {
@@ -8105,9 +8101,7 @@ static void tsc_khz_changed(void *data)
 static void kvm_hyperv_tsc_notifier(void)
 {
 	struct kvm *kvm;
-	struct kvm_vcpu *vcpu;
 	int cpu;
-	unsigned long flags;
 
 	mutex_lock(&kvm_lock);
 	list_for_each_entry(kvm, &vm_list, vm_list)
@@ -8121,19 +8115,11 @@ static void kvm_hyperv_tsc_notifier(void)
 	kvm_max_guest_tsc_khz = tsc_khz;
 
 	list_for_each_entry(kvm, &vm_list, vm_list) {
-		struct kvm_arch *ka = &kvm->arch;
-
-		kvm_make_mclock_inprogress_request(kvm);
-		spin_lock_irqsave(&ka->pvclock_gtod_sync_lock, flags);
+		kvm_start_pvclock_update(kvm);
 		pvclock_update_vm_gtod_copy(kvm);
-		spin_unlock_irqrestore(&ka->pvclock_gtod_sync_lock, flags);
-
-		kvm_for_each_vcpu(cpu, vcpu, kvm)
-			kvm_make_request(KVM_REQ_CLOCK_UPDATE, vcpu);
-
-		kvm_for_each_vcpu(cpu, vcpu, kvm)
-			kvm_clear_request(KVM_REQ_MCLOCK_INPROGRESS, vcpu);
+		kvm_end_pvclock_update(kvm);
 	}
+
 	mutex_unlock(&kvm_lock);
 }
 #endif
@@ -9413,7 +9399,7 @@ static int vcpu_enter_guest(struct kvm_vcpu *vcpu)
 		if (kvm_check_request(KVM_REQ_MIGRATE_TIMER, vcpu))
 			__kvm_migrate_timers(vcpu);
 		if (kvm_check_request(KVM_REQ_MASTERCLOCK_UPDATE, vcpu))
-			kvm_gen_update_masterclock(vcpu->kvm);
+			kvm_update_masterclock(vcpu->kvm);
 		if (kvm_check_request(KVM_REQ_GLOBAL_CLOCK_UPDATE, vcpu))
 			kvm_gen_kvmclock_update(vcpu);
 		if (kvm_check_request(KVM_REQ_CLOCK_UPDATE, vcpu)) {
-- 
2.27.0

Re: [PATCH 1/2] KVM: KVM-on-hyperv: shorten no-entry section on reenlightenment

[Marcelo Tosatti]

On Wed, Aug 11, 2021 at 06:23:55AM -0400, Paolo Bonzini wrote:
> During re-enlightenment, update kvmclock a VM at a time instead of
> raising KVM_REQ_MASTERCLOCK_UPDATE for all VMs.  Because the guests
> can now run after TSC emulation has been disabled, invalidate
> their TSC page so that they refer to the reference time counter
> MSR while the update is in progress.
> 
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>  arch/x86/kvm/x86.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> index bab8eb3e0a47..284afaa1db86 100644
> --- a/arch/x86/kvm/x86.c
> +++ b/arch/x86/kvm/x86.c
> @@ -8111,7 +8111,7 @@ static void kvm_hyperv_tsc_notifier(void)
>  
>  	mutex_lock(&kvm_lock);
>  	list_for_each_entry(kvm, &vm_list, vm_list)
> -		kvm_make_mclock_inprogress_request(kvm);
> +		kvm_hv_invalidate_tsc_page(kvm);
>  
>  	hyperv_stop_tsc_emulation();

        hyperv_stop_tsc_emulation();

        /* TSC frequency always matches when on Hyper-V */
        for_each_present_cpu(cpu)
                per_cpu(cpu_tsc_khz, cpu) = tsc_khz;
        kvm_max_guest_tsc_khz = tsc_khz; 

Is this safe with a running guest? Why?

Re: [PATCH 1/2] KVM: KVM-on-hyperv: shorten no-entry section on reenlightenment

[Paolo Bonzini]

On 11/08/21 20:03, Marcelo Tosatti wrote:
>          hyperv_stop_tsc_emulation();
> 
>          /* TSC frequency always matches when on Hyper-V */
>          for_each_present_cpu(cpu)
>                  per_cpu(cpu_tsc_khz, cpu) = tsc_khz;
>          kvm_max_guest_tsc_khz = tsc_khz;

Yeah, it's more complicated than this.  The right sequence is:

- update the master clock

- update the TSC page parameters

- stop TSC emulation

There is no need to invalidate the TSC page.

Related to this, after kvm_hv_invalidate_tsc_page the sequence value in 
the Hyper-V TSC page will always be 1, which is wrong.  I'll take a look 
at that too.

Paolo