[PATCH 0/2] KVM: VMX: Tscdeadline timer emulation fastpath

[PATCH 0/2] KVM: VMX: Tscdeadline timer emulation fastpath

[Wanpeng Li]

IPI and Timer cause the main vmexits in cloud environment observation, 
after single target IPI fastpath, let's optimize tscdeadline timer 
latency by introducing tscdeadline timer emulation fastpath , it will 
skip various KVM related checks when possible. i.e. after vmexit due 
to tscdeadline timer emulation, handle it and vmentry immediately 
without checking various kvm stuff when possible. 

Testing on SKX Server.

cyclictest in guest(w/o mwait exposed, adaptive advance lapic timer is default -1):

5632.75ns -> 4559.25ns, 19%

kvm-unit-test/vmexit.flat:

w/o APICv, w/o advance timer:
tscdeadline_immed: 4780.75 -> 3851    19.4%
tscdeadline:       7474    -> 6528.5  12.7%

w/o APICv, w/ adaptive advance timer default -1:
tscdeadline_immed: 4845.75 -> 3930.5  18.9%
tscdeadline:       6048    -> 5871.75    3%

w/ APICv, w/o avanced timer:
tscdeadline_immed: 2919    -> 2467.75 15.5%
tscdeadline:       5661.75 -> 5188.25  8.4%

w/ APICv, w/ adaptive advance timer default -1:
tscdeadline_immed: 3018.5  -> 2561    15.2%
tscdeadline:       4663.75 -> 4626.5     1%

Tested-by: Haiwei Li <lihaiwei@tencent.com>
Cc: Haiwei Li <lihaiwei@tencent.com>

Wanpeng Li (2):
  KVM: X86: TSCDEADLINE MSR emulation fastpath
  KVM: VMX: Handle preemption timer fastpath

 arch/x86/include/asm/kvm_host.h |   3 ++
 arch/x86/kvm/lapic.c            |  20 ++------
 arch/x86/kvm/lapic.h            |  16 +++++++
 arch/x86/kvm/vmx/vmx.c          | 101 ++++++++++++++++++++++++++++++++++++++--
 arch/x86/kvm/x86.c              |  55 +++++++++++++++++++---
 5 files changed, 167 insertions(+), 28 deletions(-)

-- 
2.7.4

[PATCH 1/2] KVM: X86: TSCDEADLINE MSR emulation fastpath

[Wanpeng Li]

From: Wanpeng Li <wanpengli@tencent.com>

This patch implements tscdealine msr emulation fastpath, after wrmsr 
tscdeadline vmexit, handle it as soon as possible and vmentry immediately 
without checking various kvm stuff when possible.

Tested-by: Haiwei Li <lihaiwei@tencent.com>
Cc: Haiwei Li <lihaiwei@tencent.com>
Signed-off-by: Wanpeng Li <wanpengli@tencent.com>
---
 arch/x86/include/asm/kvm_host.h |  3 +++
 arch/x86/kvm/lapic.c            | 20 +++-----------
 arch/x86/kvm/lapic.h            | 16 +++++++++++
 arch/x86/kvm/vmx/vmx.c          | 60 +++++++++++++++++++++++++++++++++++++----
 arch/x86/kvm/x86.c              | 55 ++++++++++++++++++++++++++++++++-----
 5 files changed, 126 insertions(+), 28 deletions(-)

diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h
index f26df2c..1626ce3 100644
--- a/arch/x86/include/asm/kvm_host.h
+++ b/arch/x86/include/asm/kvm_host.h
@@ -188,6 +188,7 @@ enum {
 enum exit_fastpath_completion {
 	EXIT_FASTPATH_NONE,
 	EXIT_FASTPATH_SKIP_EMUL_INS,
+	EXIT_FASTPATH_CONT_RUN,
 };
 
 struct x86_emulate_ctxt;
@@ -1265,6 +1266,8 @@ struct kvm_x86_ops {
 
 	bool (*apic_init_signal_blocked)(struct kvm_vcpu *vcpu);
 	int (*enable_direct_tlbflush)(struct kvm_vcpu *vcpu);
+	void (*fast_deliver_interrupt)(struct kvm_vcpu *vcpu);
+	bool (*event_needs_reinjection)(struct kvm_vcpu *vcpu);
 };
 
 struct kvm_x86_init_ops {
diff --git a/arch/x86/kvm/lapic.c b/arch/x86/kvm/lapic.c
index 38f7dc9..9e54301 100644
--- a/arch/x86/kvm/lapic.c
+++ b/arch/x86/kvm/lapic.c
@@ -91,6 +91,7 @@ static inline int __apic_test_and_clear_vector(int vec, void *bitmap)
 }
 
 struct static_key_deferred apic_hw_disabled __read_mostly;
+EXPORT_SYMBOL_GPL(apic_hw_disabled);
 struct static_key_deferred apic_sw_disabled __read_mostly;
 
 static inline int apic_enabled(struct kvm_lapic *apic)
@@ -311,21 +312,6 @@ static inline int apic_lvt_enabled(struct kvm_lapic *apic, int lvt_type)
 	return !(kvm_lapic_get_reg(apic, lvt_type) & APIC_LVT_MASKED);
 }
 
-static inline int apic_lvtt_oneshot(struct kvm_lapic *apic)
-{
-	return apic->lapic_timer.timer_mode == APIC_LVT_TIMER_ONESHOT;
-}
-
-static inline int apic_lvtt_period(struct kvm_lapic *apic)
-{
-	return apic->lapic_timer.timer_mode == APIC_LVT_TIMER_PERIODIC;
-}
-
-static inline int apic_lvtt_tscdeadline(struct kvm_lapic *apic)
-{
-	return apic->lapic_timer.timer_mode == APIC_LVT_TIMER_TSCDEADLINE;
-}
-
 static inline int apic_lvt_nmi_mode(u32 lvt_val)
 {
 	return (lvt_val & (APIC_MODE_MASK | APIC_LVT_MASKED)) == APIC_DM_NMI;
@@ -1781,7 +1767,7 @@ static void cancel_hv_timer(struct kvm_lapic *apic)
 	apic->lapic_timer.hv_timer_in_use = false;
 }
 
-static bool start_hv_timer(struct kvm_lapic *apic)
+bool kvm_start_hv_timer(struct kvm_lapic *apic)
 {
 	struct kvm_timer *ktimer = &apic->lapic_timer;
 	struct kvm_vcpu *vcpu = apic->vcpu;
@@ -1847,7 +1833,7 @@ static void restart_apic_timer(struct kvm_lapic *apic)
 	if (!apic_lvtt_period(apic) && atomic_read(&apic->lapic_timer.pending))
 		goto out;
 
-	if (!start_hv_timer(apic))
+	if (!kvm_start_hv_timer(apic))
 		start_sw_timer(apic);
 out:
 	preempt_enable();
diff --git a/arch/x86/kvm/lapic.h b/arch/x86/kvm/lapic.h
index 7f15f9e..4c917fd 100644
--- a/arch/x86/kvm/lapic.h
+++ b/arch/x86/kvm/lapic.h
@@ -251,6 +251,7 @@ void kvm_lapic_expired_hv_timer(struct kvm_vcpu *vcpu);
 bool kvm_lapic_hv_timer_in_use(struct kvm_vcpu *vcpu);
 void kvm_lapic_restart_hv_timer(struct kvm_vcpu *vcpu);
 bool kvm_can_post_timer_interrupt(struct kvm_vcpu *vcpu);
+bool kvm_start_hv_timer(struct kvm_lapic *apic);
 
 static inline enum lapic_mode kvm_apic_mode(u64 apic_base)
 {
@@ -262,4 +263,19 @@ static inline u8 kvm_xapic_id(struct kvm_lapic *apic)
 	return kvm_lapic_get_reg(apic, APIC_ID) >> 24;
 }
 
+static inline int apic_lvtt_oneshot(struct kvm_lapic *apic)
+{
+	return apic->lapic_timer.timer_mode == APIC_LVT_TIMER_ONESHOT;
+}
+
+static inline int apic_lvtt_period(struct kvm_lapic *apic)
+{
+	return apic->lapic_timer.timer_mode == APIC_LVT_TIMER_PERIODIC;
+}
+
+static inline int apic_lvtt_tscdeadline(struct kvm_lapic *apic)
+{
+	return apic->lapic_timer.timer_mode == APIC_LVT_TIMER_TSCDEADLINE;
+}
+
 #endif
diff --git a/arch/x86/kvm/vmx/vmx.c b/arch/x86/kvm/vmx/vmx.c
index 766303b..7688e40 100644
--- a/arch/x86/kvm/vmx/vmx.c
+++ b/arch/x86/kvm/vmx/vmx.c
@@ -6559,6 +6559,54 @@ void vmx_update_host_rsp(struct vcpu_vmx *vmx, unsigned long host_rsp)
 	}
 }
 
+static bool vmx_event_needs_reinjection(struct kvm_vcpu *vcpu)
+{
+	return (to_vmx(vcpu)->idt_vectoring_info & VECTORING_INFO_VALID_MASK) &&
+		kvm_event_needs_reinjection(vcpu);
+}
+
+static void vmx_fast_deliver_interrupt(struct kvm_vcpu *vcpu)
+{
+	u32 reg;
+	int vector;
+	struct kvm_lapic *apic = vcpu->arch.apic;
+	struct vcpu_vmx *vmx = to_vmx(vcpu);
+
+	reg = kvm_lapic_get_reg(apic, APIC_LVTT);
+	if (kvm_apic_hw_enabled(apic) && !(reg & APIC_LVT_MASKED)) {
+		vector = reg & APIC_VECTOR_MASK;
+		kvm_lapic_clear_vector(vector, apic->regs + APIC_TMR);
+
+		if (vcpu->arch.apicv_active) {
+			if (pi_test_and_set_pir(vector, &vmx->pi_desc))
+				return;
+
+			if (pi_test_and_set_on(&vmx->pi_desc))
+				return;
+
+			vmx_sync_pir_to_irr(vcpu);
+		} else {
+			kvm_lapic_set_irr(vector, apic);
+			kvm_queue_interrupt(vcpu, kvm_cpu_get_interrupt(vcpu), false);
+			vmx_inject_irq(vcpu);
+		}
+	}
+}
+
+static enum exit_fastpath_completion vmx_exit_handlers_fastpath(struct kvm_vcpu *vcpu)
+{
+	if (!is_guest_mode(vcpu)) {
+		switch (to_vmx(vcpu)->exit_reason) {
+		case EXIT_REASON_MSR_WRITE:
+			return handle_fastpath_set_msr_irqoff(vcpu);
+		default:
+			return EXIT_FASTPATH_NONE;
+		}
+	}
+
+	return EXIT_FASTPATH_NONE;
+}
+
 bool __vmx_vcpu_run(struct vcpu_vmx *vmx, unsigned long *regs, bool launched);
 
 static enum exit_fastpath_completion vmx_vcpu_run(struct kvm_vcpu *vcpu)
@@ -6566,6 +6614,7 @@ static enum exit_fastpath_completion vmx_vcpu_run(struct kvm_vcpu *vcpu)
 	enum exit_fastpath_completion exit_fastpath;
 	struct vcpu_vmx *vmx = to_vmx(vcpu);
 	unsigned long cr3, cr4;
+continue_vmx_vcpu_run:
 
 	/* Record the guest's net vcpu time for enforced NMI injections. */
 	if (unlikely(!enable_vnmi &&
@@ -6733,17 +6782,16 @@ static enum exit_fastpath_completion vmx_vcpu_run(struct kvm_vcpu *vcpu)
 	if (unlikely(vmx->exit_reason & VMX_EXIT_REASONS_FAILED_VMENTRY))
 		return EXIT_FASTPATH_NONE;
 
-	if (!is_guest_mode(vcpu) && vmx->exit_reason == EXIT_REASON_MSR_WRITE)
-		exit_fastpath = handle_fastpath_set_msr_irqoff(vcpu);
-	else
-		exit_fastpath = EXIT_FASTPATH_NONE;
-
 	vmx->loaded_vmcs->launched = 1;
 	vmx->idt_vectoring_info = vmcs_read32(IDT_VECTORING_INFO_FIELD);
 
 	vmx_recover_nmi_blocking(vmx);
 	vmx_complete_interrupts(vmx);
 
+	exit_fastpath = vmx_exit_handlers_fastpath(vcpu);
+	if (exit_fastpath == EXIT_FASTPATH_CONT_RUN)
+		goto continue_vmx_vcpu_run;
+
 	return exit_fastpath;
 }
 
@@ -7885,6 +7933,8 @@ static struct kvm_x86_ops vmx_x86_ops __initdata = {
 	.nested_get_evmcs_version = NULL,
 	.need_emulation_on_page_fault = vmx_need_emulation_on_page_fault,
 	.apic_init_signal_blocked = vmx_apic_init_signal_blocked,
+	.fast_deliver_interrupt = vmx_fast_deliver_interrupt,
+	.event_needs_reinjection = vmx_event_needs_reinjection,
 };
 
 static __init int hardware_setup(void)
diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index 59958ce..9c6733d 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -1609,27 +1609,70 @@ static int handle_fastpath_set_x2apic_icr_irqoff(struct kvm_vcpu *vcpu, u64 data
 	return 1;
 }
 
+static int handle_fastpath_set_tscdeadline(struct kvm_vcpu *vcpu, u64 data)
+{
+	struct kvm_lapic *apic = vcpu->arch.apic;
+
+	if (!lapic_in_kernel(vcpu) || apic_lvtt_oneshot(apic) ||
+			apic_lvtt_period(apic))
+		return 0;
+
+	if (!kvm_x86_ops.set_hv_timer ||
+		kvm_mwait_in_guest(vcpu->kvm) ||
+		kvm_can_post_timer_interrupt(vcpu))
+		return 1;
+
+	hrtimer_cancel(&apic->lapic_timer.timer);
+	apic->lapic_timer.tscdeadline = data;
+	atomic_set(&apic->lapic_timer.pending, 0);
+
+	if (kvm_start_hv_timer(apic)) {
+		if (kvm_check_request(KVM_REQ_PENDING_TIMER, vcpu)) {
+			if (kvm_x86_ops.interrupt_allowed(vcpu)) {
+				kvm_clear_request(KVM_REQ_PENDING_TIMER, vcpu);
+				kvm_x86_ops.fast_deliver_interrupt(vcpu);
+				atomic_set(&apic->lapic_timer.pending, 0);
+				apic->lapic_timer.tscdeadline = 0;
+				return 0;
+			}
+			return 1;
+		}
+		return 0;
+	}
+
+	return 1;
+}
+
 enum exit_fastpath_completion handle_fastpath_set_msr_irqoff(struct kvm_vcpu *vcpu)
 {
 	u32 msr = kvm_rcx_read(vcpu);
 	u64 data;
-	int ret = 0;
+	int ret = EXIT_FASTPATH_NONE;
 
 	switch (msr) {
 	case APIC_BASE_MSR + (APIC_ICR >> 4):
 		data = kvm_read_edx_eax(vcpu);
-		ret = handle_fastpath_set_x2apic_icr_irqoff(vcpu, data);
+		if (!handle_fastpath_set_x2apic_icr_irqoff(vcpu, data))
+			ret = EXIT_FASTPATH_SKIP_EMUL_INS;
+		break;
+	case MSR_IA32_TSCDEADLINE:
+		if (!kvm_x86_ops.event_needs_reinjection(vcpu)) {
+			data = kvm_read_edx_eax(vcpu);
+			if (!handle_fastpath_set_tscdeadline(vcpu, data))
+				ret = EXIT_FASTPATH_CONT_RUN;
+		}
 		break;
 	default:
-		return EXIT_FASTPATH_NONE;
+		ret = EXIT_FASTPATH_NONE;
 	}
 
-	if (!ret) {
+	if (ret != EXIT_FASTPATH_NONE) {
 		trace_kvm_msr_write(msr, data);
-		return EXIT_FASTPATH_SKIP_EMUL_INS;
+		if (ret == EXIT_FASTPATH_CONT_RUN)
+			kvm_skip_emulated_instruction(vcpu);
 	}
 
-	return EXIT_FASTPATH_NONE;
+	return ret;
 }
 EXPORT_SYMBOL_GPL(handle_fastpath_set_msr_irqoff);
 
-- 
2.7.4

[PATCH 2/2] KVM: VMX: Handle preemption timer fastpath

[Wanpeng Li]

From: Wanpeng Li <wanpengli@tencent.com>

This patch implements handle preemption timer fastpath, after timer fire 
due to VMX-preemption timer counts down to zero, handle it as soon as 
possible and vmentry immediately without checking various kvm stuff when 
possible.

Testing on SKX Server.

cyclictest in guest(w/o mwait exposed, adaptive advance lapic timer is default -1):

5632.75ns -> 4559.25ns, 19%

kvm-unit-test/vmexit.flat:

w/o APICv, w/o advance timer:
tscdeadline_immed: 4780.75 -> 3851    19.4%
tscdeadline:       7474    -> 6528.5  12.7%

w/o APICv, w/ adaptive advance timer default -1:
tscdeadline_immed: 4845.75 -> 3930.5  18.9%
tscdeadline:       6048    -> 5871.75    3%

w/ APICv, w/o avanced timer:
tscdeadline_immed: 2919    -> 2467.75 15.5%
tscdeadline:       5661.75 -> 5188.25  8.4%

w/ APICv, w/ adaptive advance timer default -1:
tscdeadline_immed: 3018.5  -> 2561    15.2%
tscdeadline:       4663.75 -> 4626.5     1%

Tested-by: Haiwei Li <lihaiwei@tencent.com>
Cc: Haiwei Li <lihaiwei@tencent.com>
Signed-off-by: Wanpeng Li <wanpengli@tencent.com>
---
 arch/x86/kvm/vmx/vmx.c | 41 +++++++++++++++++++++++++++++++++++++++++
 1 file changed, 41 insertions(+)

diff --git a/arch/x86/kvm/vmx/vmx.c b/arch/x86/kvm/vmx/vmx.c
index 7688e40..623c4a0 100644
--- a/arch/x86/kvm/vmx/vmx.c
+++ b/arch/x86/kvm/vmx/vmx.c
@@ -6593,12 +6593,53 @@ static void vmx_fast_deliver_interrupt(struct kvm_vcpu *vcpu)
 	}
 }
 
+static void vmx_cancel_hv_timer(struct kvm_vcpu *vcpu);
+
+static enum exit_fastpath_completion handle_fastpath_preemption_timer(struct kvm_vcpu *vcpu)
+{
+	struct vcpu_vmx *vmx = to_vmx(vcpu);
+	struct kvm_lapic *apic = vcpu->arch.apic;
+	struct kvm_timer *ktimer = &apic->lapic_timer;
+
+	if (vmx_event_needs_reinjection(vcpu))
+		return EXIT_FASTPATH_NONE;
+
+	if (!vmx->req_immediate_exit &&
+		!unlikely(vmx->loaded_vmcs->hv_timer_soft_disabled)) {
+		if (!vmx_interrupt_allowed(vcpu) ||
+			!apic_lvtt_tscdeadline(apic) ||
+			vmx->rmode.vm86_active ||
+			is_smm(vcpu) ||
+			!kvm_apic_hw_enabled(apic))
+			return EXIT_FASTPATH_NONE;
+
+		if (!apic->lapic_timer.hv_timer_in_use)
+			return EXIT_FASTPATH_CONT_RUN;
+
+		WARN_ON(swait_active(&vcpu->wq));
+		vmx_cancel_hv_timer(vcpu);
+		apic->lapic_timer.hv_timer_in_use = false;
+
+		if (atomic_read(&apic->lapic_timer.pending))
+			return EXIT_FASTPATH_CONT_RUN;
+
+		ktimer->expired_tscdeadline = ktimer->tscdeadline;
+		vmx_fast_deliver_interrupt(vcpu);
+		ktimer->tscdeadline = 0;
+		return EXIT_FASTPATH_CONT_RUN;
+	}
+
+	return EXIT_FASTPATH_NONE;
+}
+
 static enum exit_fastpath_completion vmx_exit_handlers_fastpath(struct kvm_vcpu *vcpu)
 {
 	if (!is_guest_mode(vcpu)) {
 		switch(to_vmx(vcpu)->exit_reason) {
 		case EXIT_REASON_MSR_WRITE:
 			return handle_fastpath_set_msr_irqoff(vcpu);
+		case EXIT_REASON_PREEMPTION_TIMER:
+			return handle_fastpath_preemption_timer(vcpu);
 		default:
 			return EXIT_FASTPATH_NONE;
 		}
-- 
2.7.4

Re: [PATCH 1/2] KVM: X86: TSCDEADLINE MSR emulation fastpath

[Paolo Bonzini]

On 21/04/20 13:20, Wanpeng Li wrote:
> +	case MSR_IA32_TSCDEADLINE:
> +		if (!kvm_x86_ops.event_needs_reinjection(vcpu)) {
> +			data = kvm_read_edx_eax(vcpu);
> +			if (!handle_fastpath_set_tscdeadline(vcpu, data))
> +				ret = EXIT_FASTPATH_CONT_RUN;
> +		}
>  		break;

Can you explain the event_needs_reinjection case?  Also, does this break
AMD which does not implement the callback?

> +
> +	reg = kvm_lapic_get_reg(apic, APIC_LVTT);
> +	if (kvm_apic_hw_enabled(apic) && !(reg & APIC_LVT_MASKED)) {
> +		vector = reg & APIC_VECTOR_MASK;
> +		kvm_lapic_clear_vector(vector, apic->regs + APIC_TMR);
> +
> +		if (vcpu->arch.apicv_active) {
> +			if (pi_test_and_set_pir(vector, &vmx->pi_desc))
> +				return;
> +
> +			if (pi_test_and_set_on(&vmx->pi_desc))
> +				return;
> +
> +			vmx_sync_pir_to_irr(vcpu);
> +		} else {
> +			kvm_lapic_set_irr(vector, apic);
> +			kvm_queue_interrupt(vcpu, kvm_cpu_get_interrupt(vcpu), false);
> +			vmx_inject_irq(vcpu);
> +		}
> +	}

This is mostly a copy of

               if (kvm_x86_ops.deliver_posted_interrupt(vcpu, vector)) {
                        kvm_lapic_set_irr(vector, apic);
                        kvm_make_request(KVM_REQ_EVENT, vcpu);
                        kvm_vcpu_kick(vcpu);
                }
                break;

(is it required to do vmx_sync_pir_to_irr?).  So you should not special
case LVTT and move this code to lapic.c instead.  But even before that...

> 
> +
> +	if (kvm_start_hv_timer(apic)) {
> +		if (kvm_check_request(KVM_REQ_PENDING_TIMER, vcpu)) {
> +			if (kvm_x86_ops.interrupt_allowed(vcpu)) {
> +				kvm_clear_request(KVM_REQ_PENDING_TIMER, vcpu);
> +				kvm_x86_ops.fast_deliver_interrupt(vcpu);
> +				atomic_set(&apic->lapic_timer.pending, 0);
> +				apic->lapic_timer.tscdeadline = 0;
> +				return 0;
> +			}
> +			return 1;


Is it actually common that the timer is set back in time and therefore
this code is executed?

Paolo

Re: [PATCH 2/2] KVM: VMX: Handle preemption timer fastpath

[Paolo Bonzini]

On 21/04/20 13:20, Wanpeng Li wrote:
> +
> +	if (!vmx->req_immediate_exit &&
> +		!unlikely(vmx->loaded_vmcs->hv_timer_soft_disabled)) {
> +		if (!vmx_interrupt_allowed(vcpu) ||
> +			!apic_lvtt_tscdeadline(apic) ||
> +			vmx->rmode.vm86_active ||
> +			is_smm(vcpu) ||
> +			!kvm_apic_hw_enabled(apic))
> +			return EXIT_FASTPATH_NONE;
> +
> +		if (!apic->lapic_timer.hv_timer_in_use)
> +			return EXIT_FASTPATH_CONT_RUN;
> +
> +		WARN_ON(swait_active(&vcpu->wq));
> +		vmx_cancel_hv_timer(vcpu);
> +		apic->lapic_timer.hv_timer_in_use = false;
> +
> +		if (atomic_read(&apic->lapic_timer.pending))
> +			return EXIT_FASTPATH_CONT_RUN;
> +
> +		ktimer->expired_tscdeadline = ktimer->tscdeadline;
> +		vmx_fast_deliver_interrupt(vcpu);
> +		ktimer->tscdeadline = 0;
> +		return EXIT_FASTPATH_CONT_RUN;
> +	}
> +

Can you explain all the checks you have here, and why you need something
more complex than apic_timer_expired (possibly by adding some
optimizations to kvm_apic_local_deliver)?  This code is impossible to
maintain.

Paolo

Re: [PATCH 1/2] KVM: X86: TSCDEADLINE MSR emulation fastpath

[Wanpeng Li]

On Tue, 21 Apr 2020 at 19:37, Paolo Bonzini <pbonzini@redhat.com> wrote:
>
> On 21/04/20 13:20, Wanpeng Li wrote:
> > +     case MSR_IA32_TSCDEADLINE:
> > +             if (!kvm_x86_ops.event_needs_reinjection(vcpu)) {
> > +                     data = kvm_read_edx_eax(vcpu);
> > +                     if (!handle_fastpath_set_tscdeadline(vcpu, data))
> > +                             ret = EXIT_FASTPATH_CONT_RUN;
> > +             }
> >               break;
>
> Can you explain the event_needs_reinjection case?  Also, does this break

This is used to catch the case vmexit occurred while another event was
being delivered to guest software, I move the
vmx_exit_handlers_fastpath() call after vmx_complete_interrupts()
which will decode such event and make kvm_event_needs_reinjection
return true.

> AMD which does not implement the callback?

Now I add the tscdeadline msr emulation and vmx-preemption timer
fastpath pair for Intel platform.

>
> > +
> > +     reg = kvm_lapic_get_reg(apic, APIC_LVTT);
> > +     if (kvm_apic_hw_enabled(apic) && !(reg & APIC_LVT_MASKED)) {
> > +             vector = reg & APIC_VECTOR_MASK;
> > +             kvm_lapic_clear_vector(vector, apic->regs + APIC_TMR);
> > +
> > +             if (vcpu->arch.apicv_active) {
> > +                     if (pi_test_and_set_pir(vector, &vmx->pi_desc))
> > +                             return;
> > +
> > +                     if (pi_test_and_set_on(&vmx->pi_desc))
> > +                             return;
> > +
> > +                     vmx_sync_pir_to_irr(vcpu);
> > +             } else {
> > +                     kvm_lapic_set_irr(vector, apic);
> > +                     kvm_queue_interrupt(vcpu, kvm_cpu_get_interrupt(vcpu), false);
> > +                     vmx_inject_irq(vcpu);
> > +             }
> > +     }
>
> This is mostly a copy of
>
>                if (kvm_x86_ops.deliver_posted_interrupt(vcpu, vector)) {
>                         kvm_lapic_set_irr(vector, apic);
>                         kvm_make_request(KVM_REQ_EVENT, vcpu);
>                         kvm_vcpu_kick(vcpu);
>                 }
>                 break;
>
> (is it required to do vmx_sync_pir_to_irr?).  So you should not special

I observe send notification vector as in
kvm_x86_ops.deliver_posted_interrupt() is ~900 cycles worse than
vmx_sync_pir_to_irr in my case. It needs to wait guest vmentry, then
the physical cpu ack the notification vector, read posted-interrupt
desciptor etc. For the non-APICv part, original copy needs to wait
inject_pending_event to do these stuff.

> case LVTT and move this code to lapic.c instead.  But even before that...
>
> >
> > +
> > +     if (kvm_start_hv_timer(apic)) {
> > +             if (kvm_check_request(KVM_REQ_PENDING_TIMER, vcpu)) {
> > +                     if (kvm_x86_ops.interrupt_allowed(vcpu)) {
> > +                             kvm_clear_request(KVM_REQ_PENDING_TIMER, vcpu);
> > +                             kvm_x86_ops.fast_deliver_interrupt(vcpu);
> > +                             atomic_set(&apic->lapic_timer.pending, 0);
> > +                             apic->lapic_timer.tscdeadline = 0;
> > +                             return 0;
> > +                     }
> > +                     return 1;
>
>
> Is it actually common that the timer is set back in time and therefore
> this code is executed?

It is used to handle the already-expired timer.

    Wanpeng

Re: [PATCH 2/2] KVM: VMX: Handle preemption timer fastpath

[Wanpeng Li]

On Tue, 21 Apr 2020 at 19:40, Paolo Bonzini <pbonzini@redhat.com> wrote:
>
> On 21/04/20 13:20, Wanpeng Li wrote:
> > +
> > +     if (!vmx->req_immediate_exit &&
> > +             !unlikely(vmx->loaded_vmcs->hv_timer_soft_disabled)) {
> > +             if (!vmx_interrupt_allowed(vcpu) ||

For non-APICv case, we need to request interrupt-window.

> > +                     !apic_lvtt_tscdeadline(apic) ||

Now just add fastpath for tscdeadline mode.

> > +                     vmx->rmode.vm86_active ||
> > +                     is_smm(vcpu) ||
> > +                     !kvm_apic_hw_enabled(apic))

These stuff can be removed, kvm_apic_hw_enable() is check in
vmx_fast_deliver_interrupt().

> > +                     return EXIT_FASTPATH_NONE;
> > +
> > +             if (!apic->lapic_timer.hv_timer_in_use)
> > +                     return EXIT_FASTPATH_CONT_RUN;
> > +
> > +             WARN_ON(swait_active(&vcpu->wq));
> > +             vmx_cancel_hv_timer(vcpu);
> > +             apic->lapic_timer.hv_timer_in_use = false;
> > +
> > +             if (atomic_read(&apic->lapic_timer.pending))
> > +                     return EXIT_FASTPATH_CONT_RUN;

Other two checks are the same in kvm_lapic_expired_hv_timer().

    Wanpeng

> > +
> > +             ktimer->expired_tscdeadline = ktimer->tscdeadline;
> > +             vmx_fast_deliver_interrupt(vcpu);
> > +             ktimer->tscdeadline = 0;
> > +             return EXIT_FASTPATH_CONT_RUN;
> > +     }
> > +
>
> Can you explain all the checks you have here, and why you need something
> more complex than apic_timer_expired (possibly by adding some
> optimizations to kvm_apic_local_deliver)?  This code is impossible to
> maintain.
>
> Paolo
>

Re: [PATCH 1/2] KVM: X86: TSCDEADLINE MSR emulation fastpath

[Paolo Bonzini]

On 22/04/20 02:48, Wanpeng Li wrote:
> On Tue, 21 Apr 2020 at 19:37, Paolo Bonzini <pbonzini@redhat.com> wrote:
>> On 21/04/20 13:20, Wanpeng Li wrote:
>>> +     case MSR_IA32_TSCDEADLINE:
>>> +             if (!kvm_x86_ops.event_needs_reinjection(vcpu)) {
>>> +                     data = kvm_read_edx_eax(vcpu);
>>> +                     if (!handle_fastpath_set_tscdeadline(vcpu, data))
>>> +                             ret = EXIT_FASTPATH_CONT_RUN;
>>> +             }
>>>               break;
>> Can you explain the event_needs_reinjection case?  Also, does this break
> This is used to catch the case vmexit occurred while another event was
> being delivered to guest software, I move the
> vmx_exit_handlers_fastpath() call after vmx_complete_interrupts()
> which will decode such event and make kvm_event_needs_reinjection
> return true.

This doesn't need a callback, kvm_event_needs_reinjection should be enough.

You should also check other conditions such as

	vcpu->mode == EXITING_GUEST_MODE || kvm_request_pending(vcpu)
            || need_resched() || signal_pending(current)

before doing CONT_RUN.

>> AMD which does not implement the callback?
> Now I add the tscdeadline msr emulation and vmx-preemption timer
> fastpath pair for Intel platform.

But this would cause a crash if you run on AMD.

Paolo

Re: [PATCH 1/2] KVM: X86: TSCDEADLINE MSR emulation fastpath

[Wanpeng Li]

On Wed, 22 Apr 2020 at 16:38, Paolo Bonzini <pbonzini@redhat.com> wrote:
>
> On 22/04/20 02:48, Wanpeng Li wrote:
> > On Tue, 21 Apr 2020 at 19:37, Paolo Bonzini <pbonzini@redhat.com> wrote:
> >> On 21/04/20 13:20, Wanpeng Li wrote:
> >>> +     case MSR_IA32_TSCDEADLINE:
> >>> +             if (!kvm_x86_ops.event_needs_reinjection(vcpu)) {
> >>> +                     data = kvm_read_edx_eax(vcpu);
> >>> +                     if (!handle_fastpath_set_tscdeadline(vcpu, data))
> >>> +                             ret = EXIT_FASTPATH_CONT_RUN;
> >>> +             }
> >>>               break;
> >> Can you explain the event_needs_reinjection case?  Also, does this break
> > This is used to catch the case vmexit occurred while another event was
> > being delivered to guest software, I move the
> > vmx_exit_handlers_fastpath() call after vmx_complete_interrupts()
> > which will decode such event and make kvm_event_needs_reinjection
> > return true.
>
> This doesn't need a callback, kvm_event_needs_reinjection should be enough.
>
> You should also check other conditions such as
>
>         vcpu->mode == EXITING_GUEST_MODE || kvm_request_pending(vcpu)
>             || need_resched() || signal_pending(current)
>
> before doing CONT_RUN.

Agreed.

>
> >> AMD which does not implement the callback?
> > Now I add the tscdeadline msr emulation and vmx-preemption timer
> > fastpath pair for Intel platform.
>
> But this would cause a crash if you run on AMD.

Yes. :)

    Wanpeng