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):
5540.5ns -> 4602ns 17%
kvm-unit-test/vmexit.flat:
w/o avanced timer:
tscdeadline_immed: 2885 -> 2431.25 15.7%
tscdeadline: 5668.75 -> 5188.5 8.4%
w/ adaptive advance timer default -1:
tscdeadline_immed: 2965.25 -> 2520 15.0%
tscdeadline: 4663.75 -> 4537 2.7%
Tested-by: Haiwei Li <[email protected]>
Cc: Haiwei Li <[email protected]>
v2 -> v3:
* skip interrupt notify and use vmx_sync_pir_to_irr before each cont_run
* add from_timer_fn argument to apic_timer_expired
* remove all kinds of duplicate codes
v1 -> v2:
* move more stuff from vmx.c to lapic.c
* remove redundant checking
* check more conditions to bail out CONT_RUN
* not break AMD
* not handle LVTT sepecial
* cleanup codes
Wanpeng Li (5):
KVM: VMX: Introduce generic fastpath handler
KVM: X86: Introduce need_cancel_enter_guest helper
KVM: VMX: Optimize posted-interrupt delivery for timer fastpath
KVM: X86: TSCDEADLINE MSR emulation fastpath
KVM: VMX: Handle preemption timer fastpath
arch/x86/include/asm/kvm_host.h | 1 +
arch/x86/kvm/lapic.c | 18 +++++++++-----
arch/x86/kvm/vmx/vmx.c | 52 ++++++++++++++++++++++++++++++++++-------
arch/x86/kvm/x86.c | 40 ++++++++++++++++++++++++-------
arch/x86/kvm/x86.h | 1 +
virt/kvm/kvm_main.c | 1 +
6 files changed, 91 insertions(+), 22 deletions(-)
--
2.7.4
From: Wanpeng Li <[email protected]>
Introduce generic fastpath handler to handle MSR fastpath, VMX-preemption
timer fastpath etc. In addition, we can't observe benefit from single
target IPI fastpath when APICv is disabled, let's just enable IPI and
Timer fastpath when APICv is enabled for now.
Tested-by: Haiwei Li <[email protected]>
Cc: Haiwei Li <[email protected]>
Signed-off-by: Wanpeng Li <[email protected]>
---
arch/x86/include/asm/kvm_host.h | 1 +
arch/x86/kvm/vmx/vmx.c | 25 ++++++++++++++++++++-----
2 files changed, 21 insertions(+), 5 deletions(-)
diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h
index f26df2c..6397723 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;
diff --git a/arch/x86/kvm/vmx/vmx.c b/arch/x86/kvm/vmx/vmx.c
index 766303b..f1f6638 100644
--- a/arch/x86/kvm/vmx/vmx.c
+++ b/arch/x86/kvm/vmx/vmx.c
@@ -6559,6 +6559,20 @@ void vmx_update_host_rsp(struct vcpu_vmx *vmx, unsigned long host_rsp)
}
}
+static enum exit_fastpath_completion vmx_exit_handlers_fastpath(struct kvm_vcpu *vcpu)
+{
+ if (!is_guest_mode(vcpu) && vcpu->arch.apicv_active) {
+ 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)
@@ -6567,6 +6581,7 @@ static enum exit_fastpath_completion vmx_vcpu_run(struct kvm_vcpu *vcpu)
struct vcpu_vmx *vmx = to_vmx(vcpu);
unsigned long cr3, cr4;
+cont_run:
/* Record the guest's net vcpu time for enforced NMI injections. */
if (unlikely(!enable_vnmi &&
vmx->loaded_vmcs->soft_vnmi_blocked))
@@ -6733,17 +6748,17 @@ 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);
+ /* static call is better with retpolines */
+ if (exit_fastpath == EXIT_FASTPATH_CONT_RUN)
+ goto cont_run;
+
return exit_fastpath;
}
--
2.7.4
From: Wanpeng Li <[email protected]>
Optimizing posted-interrupt delivery especially for the timer fastpath
scenario, I observe kvm_x86_ops.deliver_posted_interrupt() has more latency
then vmx_sync_pir_to_irr() in the case of timer fastpath scenario, since
it needs to wait vmentry, after that it can handle external interrupt, ack
the notification vector, read posted-interrupt descriptor etc, it is slower
than evaluate and delivery during vmentry immediately approach. Let's skip
sending interrupt to notify target pCPU and replace by vmx_sync_pir_to_irr()
before each cont_run.
Tested-by: Haiwei Li <[email protected]>
Cc: Haiwei Li <[email protected]>
Signed-off-by: Wanpeng Li <[email protected]>
---
arch/x86/kvm/vmx/vmx.c | 9 ++++++---
virt/kvm/kvm_main.c | 1 +
2 files changed, 7 insertions(+), 3 deletions(-)
diff --git a/arch/x86/kvm/vmx/vmx.c b/arch/x86/kvm/vmx/vmx.c
index 5c21027..d21b66b 100644
--- a/arch/x86/kvm/vmx/vmx.c
+++ b/arch/x86/kvm/vmx/vmx.c
@@ -3909,8 +3909,9 @@ static int vmx_deliver_posted_interrupt(struct kvm_vcpu *vcpu, int vector)
if (pi_test_and_set_on(&vmx->pi_desc))
return 0;
- if (!kvm_vcpu_trigger_posted_interrupt(vcpu, false))
- kvm_vcpu_kick(vcpu);
+ if (vcpu != kvm_get_running_vcpu() &&
+ !kvm_vcpu_trigger_posted_interrupt(vcpu, false))
+ kvm_vcpu_kick(vcpu);
return 0;
}
@@ -6757,8 +6758,10 @@ static enum exit_fastpath_completion vmx_vcpu_run(struct kvm_vcpu *vcpu)
if (!kvm_need_cancel_enter_guest(vcpu)) {
exit_fastpath = vmx_exit_handlers_fastpath(vcpu);
/* static call is better with retpolines */
- if (exit_fastpath == EXIT_FASTPATH_CONT_RUN)
+ if (exit_fastpath == EXIT_FASTPATH_CONT_RUN) {
+ vmx_sync_pir_to_irr(vcpu);
goto cont_run;
+ }
}
return exit_fastpath;
diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
index e7436d0..6a289d1 100644
--- a/virt/kvm/kvm_main.c
+++ b/virt/kvm/kvm_main.c
@@ -4633,6 +4633,7 @@ struct kvm_vcpu *kvm_get_running_vcpu(void)
return vcpu;
}
+EXPORT_SYMBOL_GPL(kvm_get_running_vcpu);
/**
* kvm_get_running_vcpus - get the per-CPU array of currently running vcpus.
--
2.7.4
From: Wanpeng Li <[email protected]>
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):
5540.5ns -> 4602ns 17%
kvm-unit-test/vmexit.flat:
w/o avanced timer:
tscdeadline_immed: 2885 -> 2431.25 15.7%
tscdeadline: 5668.75 -> 5188.5 8.4%
w/ adaptive advance timer default -1:
tscdeadline_immed: 2965.25 -> 2520 15.0%
tscdeadline: 4663.75 -> 4537 2.7%
Tested-by: Haiwei Li <[email protected]>
Cc: Haiwei Li <[email protected]>
Signed-off-by: Wanpeng Li <[email protected]>
---
arch/x86/kvm/vmx/vmx.c | 16 ++++++++++++++++
1 file changed, 16 insertions(+)
diff --git a/arch/x86/kvm/vmx/vmx.c b/arch/x86/kvm/vmx/vmx.c
index d21b66b..028967a 100644
--- a/arch/x86/kvm/vmx/vmx.c
+++ b/arch/x86/kvm/vmx/vmx.c
@@ -6560,12 +6560,28 @@ void vmx_update_host_rsp(struct vcpu_vmx *vmx, unsigned long host_rsp)
}
}
+static enum exit_fastpath_completion handle_fastpath_preemption_timer(struct kvm_vcpu *vcpu)
+{
+ struct vcpu_vmx *vmx = to_vmx(vcpu);
+
+ if (!vmx->req_immediate_exit &&
+ !unlikely(vmx->loaded_vmcs->hv_timer_soft_disabled)) {
+ kvm_lapic_expired_hv_timer(vcpu);
+ trace_kvm_exit(EXIT_REASON_PREEMPTION_TIMER, vcpu, KVM_ISA_VMX);
+ 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) && vcpu->arch.apicv_active) {
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
From: Wanpeng Li <[email protected]>
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 <[email protected]>
Cc: Haiwei Li <[email protected]>
Signed-off-by: Wanpeng Li <[email protected]>
---
arch/x86/kvm/lapic.c | 18 ++++++++++++------
arch/x86/kvm/x86.c | 30 ++++++++++++++++++++++++------
2 files changed, 36 insertions(+), 12 deletions(-)
diff --git a/arch/x86/kvm/lapic.c b/arch/x86/kvm/lapic.c
index 38f7dc9..3589237 100644
--- a/arch/x86/kvm/lapic.c
+++ b/arch/x86/kvm/lapic.c
@@ -1593,7 +1593,7 @@ static void kvm_apic_inject_pending_timer_irqs(struct kvm_lapic *apic)
}
}
-static void apic_timer_expired(struct kvm_lapic *apic)
+static void apic_timer_expired(struct kvm_lapic *apic, bool from_timer_fn)
{
struct kvm_vcpu *vcpu = apic->vcpu;
struct kvm_timer *ktimer = &apic->lapic_timer;
@@ -1604,6 +1604,12 @@ static void apic_timer_expired(struct kvm_lapic *apic)
if (apic_lvtt_tscdeadline(apic) || ktimer->hv_timer_in_use)
ktimer->expired_tscdeadline = ktimer->tscdeadline;
+ if (!from_timer_fn && vcpu->arch.apicv_active) {
+ WARN_ON(kvm_get_running_vcpu() != vcpu);
+ kvm_apic_inject_pending_timer_irqs(apic);
+ return;
+ }
+
if (kvm_use_posted_timer_interrupt(apic->vcpu)) {
if (apic->lapic_timer.timer_advance_ns)
__kvm_wait_lapic_expire(vcpu);
@@ -1643,7 +1649,7 @@ static void start_sw_tscdeadline(struct kvm_lapic *apic)
expire = ktime_sub_ns(expire, ktimer->timer_advance_ns);
hrtimer_start(&ktimer->timer, expire, HRTIMER_MODE_ABS_HARD);
} else
- apic_timer_expired(apic);
+ apic_timer_expired(apic, false);
local_irq_restore(flags);
}
@@ -1751,7 +1757,7 @@ static void start_sw_period(struct kvm_lapic *apic)
if (ktime_after(ktime_get(),
apic->lapic_timer.target_expiration)) {
- apic_timer_expired(apic);
+ apic_timer_expired(apic, false);
if (apic_lvtt_oneshot(apic))
return;
@@ -1813,7 +1819,7 @@ static bool start_hv_timer(struct kvm_lapic *apic)
if (atomic_read(&ktimer->pending)) {
cancel_hv_timer(apic);
} else if (expired) {
- apic_timer_expired(apic);
+ apic_timer_expired(apic, false);
cancel_hv_timer(apic);
}
}
@@ -1863,7 +1869,7 @@ void kvm_lapic_expired_hv_timer(struct kvm_vcpu *vcpu)
goto out;
WARN_ON(swait_active(&vcpu->wq));
cancel_hv_timer(apic);
- apic_timer_expired(apic);
+ apic_timer_expired(apic, false);
if (apic_lvtt_period(apic) && apic->lapic_timer.period) {
advance_periodic_target_expiration(apic);
@@ -2369,7 +2375,7 @@ static enum hrtimer_restart apic_timer_fn(struct hrtimer *data)
struct kvm_timer *ktimer = container_of(data, struct kvm_timer, timer);
struct kvm_lapic *apic = container_of(ktimer, struct kvm_lapic, lapic_timer);
- apic_timer_expired(apic);
+ apic_timer_expired(apic, true);
if (lapic_is_periodic(apic)) {
advance_periodic_target_expiration(apic);
diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index 4561104..99061ba 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -1616,27 +1616,45 @@ 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)
+{
+ if (!kvm_x86_ops.set_hv_timer ||
+ kvm_mwait_in_guest(vcpu->kvm) ||
+ kvm_can_post_timer_interrupt(vcpu))
+ return 1;
+
+ kvm_set_lapic_tscdeadline_msr(vcpu, data);
+ return 0;
+}
+
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:
+ 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
From: Wanpeng Li <[email protected]>
Introduce need_cancel_enter_guest() helper, we need to check some
conditions before doing CONT_RUN, in addition, it can also catch
the case vmexit occurred while another event was being delivered
to guest software since vmx_complete_interrupts() adds the request
bit.
Tested-by: Haiwei Li <[email protected]>
Cc: Haiwei Li <[email protected]>
Signed-off-by: Wanpeng Li <[email protected]>
---
arch/x86/kvm/vmx/vmx.c | 12 +++++++-----
arch/x86/kvm/x86.c | 10 ++++++++--
arch/x86/kvm/x86.h | 1 +
3 files changed, 16 insertions(+), 7 deletions(-)
diff --git a/arch/x86/kvm/vmx/vmx.c b/arch/x86/kvm/vmx/vmx.c
index f1f6638..5c21027 100644
--- a/arch/x86/kvm/vmx/vmx.c
+++ b/arch/x86/kvm/vmx/vmx.c
@@ -6577,7 +6577,7 @@ 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)
{
- enum exit_fastpath_completion exit_fastpath;
+ enum exit_fastpath_completion exit_fastpath = EXIT_FASTPATH_NONE;
struct vcpu_vmx *vmx = to_vmx(vcpu);
unsigned long cr3, cr4;
@@ -6754,10 +6754,12 @@ static enum exit_fastpath_completion vmx_vcpu_run(struct kvm_vcpu *vcpu)
vmx_recover_nmi_blocking(vmx);
vmx_complete_interrupts(vmx);
- exit_fastpath = vmx_exit_handlers_fastpath(vcpu);
- /* static call is better with retpolines */
- if (exit_fastpath == EXIT_FASTPATH_CONT_RUN)
- goto cont_run;
+ if (!kvm_need_cancel_enter_guest(vcpu)) {
+ exit_fastpath = vmx_exit_handlers_fastpath(vcpu);
+ /* static call is better with retpolines */
+ if (exit_fastpath == EXIT_FASTPATH_CONT_RUN)
+ goto cont_run;
+ }
return exit_fastpath;
}
diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index 59958ce..4561104 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -1581,6 +1581,13 @@ int kvm_emulate_wrmsr(struct kvm_vcpu *vcpu)
}
EXPORT_SYMBOL_GPL(kvm_emulate_wrmsr);
+bool kvm_need_cancel_enter_guest(struct kvm_vcpu *vcpu)
+{
+ return (vcpu->mode == EXITING_GUEST_MODE || kvm_request_pending(vcpu)
+ || need_resched() || signal_pending(current));
+}
+EXPORT_SYMBOL_GPL(kvm_need_cancel_enter_guest);
+
/*
* The fast path for frequent and performance sensitive wrmsr emulation,
* i.e. the sending of IPI, sending IPI early in the VM-Exit flow reduces
@@ -8373,8 +8380,7 @@ static int vcpu_enter_guest(struct kvm_vcpu *vcpu)
if (kvm_lapic_enabled(vcpu) && vcpu->arch.apicv_active)
kvm_x86_ops.sync_pir_to_irr(vcpu);
- if (vcpu->mode == EXITING_GUEST_MODE || kvm_request_pending(vcpu)
- || need_resched() || signal_pending(current)) {
+ if (kvm_need_cancel_enter_guest(vcpu)) {
vcpu->mode = OUTSIDE_GUEST_MODE;
smp_wmb();
local_irq_enable();
diff --git a/arch/x86/kvm/x86.h b/arch/x86/kvm/x86.h
index 7b5ed8e..1906e7e 100644
--- a/arch/x86/kvm/x86.h
+++ b/arch/x86/kvm/x86.h
@@ -364,5 +364,6 @@ static inline bool kvm_dr7_valid(u64 data)
void kvm_load_guest_xsave_state(struct kvm_vcpu *vcpu);
void kvm_load_host_xsave_state(struct kvm_vcpu *vcpu);
u64 kvm_spec_ctrl_valid_bits(struct kvm_vcpu *vcpu);
+bool kvm_need_cancel_enter_guest(struct kvm_vcpu *vcpu);
#endif
--
2.7.4
On Fri, 24 Apr 2020 at 14:23, Wanpeng Li <[email protected]> wrote:
>
> From: Wanpeng Li <[email protected]>
>
> Introduce need_cancel_enter_guest() helper, we need to check some
> conditions before doing CONT_RUN, in addition, it can also catch
> the case vmexit occurred while another event was being delivered
> to guest software since vmx_complete_interrupts() adds the request
> bit.
>
> Tested-by: Haiwei Li <[email protected]>
> Cc: Haiwei Li <[email protected]>
> Signed-off-by: Wanpeng Li <[email protected]>
> ---
> arch/x86/kvm/vmx/vmx.c | 12 +++++++-----
> arch/x86/kvm/x86.c | 10 ++++++++--
> arch/x86/kvm/x86.h | 1 +
> 3 files changed, 16 insertions(+), 7 deletions(-)
>
> diff --git a/arch/x86/kvm/vmx/vmx.c b/arch/x86/kvm/vmx/vmx.c
> index f1f6638..5c21027 100644
> --- a/arch/x86/kvm/vmx/vmx.c
> +++ b/arch/x86/kvm/vmx/vmx.c
> @@ -6577,7 +6577,7 @@ 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)
> {
> - enum exit_fastpath_completion exit_fastpath;
> + enum exit_fastpath_completion exit_fastpath = EXIT_FASTPATH_NONE;
> struct vcpu_vmx *vmx = to_vmx(vcpu);
> unsigned long cr3, cr4;
>
> @@ -6754,10 +6754,12 @@ static enum exit_fastpath_completion vmx_vcpu_run(struct kvm_vcpu *vcpu)
> vmx_recover_nmi_blocking(vmx);
> vmx_complete_interrupts(vmx);
>
> - exit_fastpath = vmx_exit_handlers_fastpath(vcpu);
> - /* static call is better with retpolines */
> - if (exit_fastpath == EXIT_FASTPATH_CONT_RUN)
> - goto cont_run;
> + if (!kvm_need_cancel_enter_guest(vcpu)) {
> + exit_fastpath = vmx_exit_handlers_fastpath(vcpu);
> + /* static call is better with retpolines */
> + if (exit_fastpath == EXIT_FASTPATH_CONT_RUN)
> + goto cont_run;
> + }
The kvm_need_cancel_enter_guest() should not before
vmx_exit_handlers_fastpath() which will break IPI fastpath. How about
applying something like below, otherwise, maybe introduce another
EXIT_FASTPATH_CONT_FAIL to indicate fails due to
kvm_need_cancel_enter_guest() if checking it after
vmx_exit_handlers_fastpath(), then we return 1 in vmx_handle_exit()
directly instead of kvm_skip_emulated_instruction(). VMX-preemption
timer exit doesn't need to skip emulated instruction but wrmsr
TSCDEADLINE MSR exit does which results in a little complex here.
Paolo, what do you think?
diff --git a/arch/x86/kvm/vmx/vmx.c b/arch/x86/kvm/vmx/vmx.c
index 853d3af..9317924 100644
--- a/arch/x86/kvm/vmx/vmx.c
+++ b/arch/x86/kvm/vmx/vmx.c
@@ -6564,6 +6564,9 @@ static enum exit_fastpath_completion
handle_fastpath_preemption_timer(struct kvm
{
struct vcpu_vmx *vmx = to_vmx(vcpu);
+ if (kvm_need_cancel_enter_guest(vcpu))
+ return EXIT_FASTPATH_NONE;
+
if (!vmx->req_immediate_exit &&
!unlikely(vmx->loaded_vmcs->hv_timer_soft_disabled)) {
kvm_lapic_expired_hv_timer(vcpu);
@@ -6771,12 +6774,10 @@ static enum exit_fastpath_completion
vmx_vcpu_run(struct kvm_vcpu *vcpu)
vmx_recover_nmi_blocking(vmx);
vmx_complete_interrupts(vmx);
- if (!(kvm_need_cancel_enter_guest(vcpu))) {
- exit_fastpath = vmx_exit_handlers_fastpath(vcpu);
- if (exit_fastpath == EXIT_FASTPATH_CONT_RUN) {
- vmx_sync_pir_to_irr(vcpu);
- goto cont_run;
- }
+ exit_fastpath = vmx_exit_handlers_fastpath(vcpu);
+ if (exit_fastpath == EXIT_FASTPATH_CONT_RUN) {
+ vmx_sync_pir_to_irr(vcpu);
+ goto cont_run;
}
return exit_fastpath;
diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index 99061ba..11b309c 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -1618,6 +1618,9 @@ static int
handle_fastpath_set_x2apic_icr_irqoff(struct kvm_vcpu *vcpu, u64 data
static int handle_fastpath_set_tscdeadline(struct kvm_vcpu *vcpu, u64 data)
{
+ if (kvm_need_cancel_enter_guest(vcpu))
+ return 1;
+
if (!kvm_x86_ops.set_hv_timer ||
kvm_mwait_in_guest(vcpu->kvm) ||
kvm_can_post_timer_interrupt(vcpu))
On Fri, Apr 24, 2020 at 02:22:40PM +0800, Wanpeng Li wrote:
> From: Wanpeng Li <[email protected]>
>
> Introduce generic fastpath handler to handle MSR fastpath, VMX-preemption
> timer fastpath etc. In addition, we can't observe benefit from single
> target IPI fastpath when APICv is disabled, let's just enable IPI and
> Timer fastpath when APICv is enabled for now.
There are three different changes being squished into a single patch:
- Refactor code to add helper
- Change !APICv behavior for WRMSR fastpath
- Introduce EXIT_FASTPATH_CONT_RUN
I don't think you necessarily need to break this into three separate
patches, but's the !APICv change needs to be a standalone patch, especially
given the shortlog. E.g. the refactoring could be introduced along with
the second fastpath case, and CONT_RUN could be introduced with its first
usage.
>
> Tested-by: Haiwei Li <[email protected]>
> Cc: Haiwei Li <[email protected]>
> Signed-off-by: Wanpeng Li <[email protected]>
> ---
> arch/x86/include/asm/kvm_host.h | 1 +
> arch/x86/kvm/vmx/vmx.c | 25 ++++++++++++++++++++-----
> 2 files changed, 21 insertions(+), 5 deletions(-)
>
> diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h
> index f26df2c..6397723 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;
> diff --git a/arch/x86/kvm/vmx/vmx.c b/arch/x86/kvm/vmx/vmx.c
> index 766303b..f1f6638 100644
> --- a/arch/x86/kvm/vmx/vmx.c
> +++ b/arch/x86/kvm/vmx/vmx.c
> @@ -6559,6 +6559,20 @@ void vmx_update_host_rsp(struct vcpu_vmx *vmx, unsigned long host_rsp)
> }
> }
>
> +static enum exit_fastpath_completion vmx_exit_handlers_fastpath(struct kvm_vcpu *vcpu)
> +{
> + if (!is_guest_mode(vcpu) && vcpu->arch.apicv_active) {
> + 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)
> @@ -6567,6 +6581,7 @@ static enum exit_fastpath_completion vmx_vcpu_run(struct kvm_vcpu *vcpu)
> struct vcpu_vmx *vmx = to_vmx(vcpu);
> unsigned long cr3, cr4;
>
> +cont_run:
> /* Record the guest's net vcpu time for enforced NMI injections. */
> if (unlikely(!enable_vnmi &&
> vmx->loaded_vmcs->soft_vnmi_blocked))
> @@ -6733,17 +6748,17 @@ 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);
> + /* static call is better with retpolines */
> + if (exit_fastpath == EXIT_FASTPATH_CONT_RUN)
> + goto cont_run;
> +
> return exit_fastpath;
> }
>
> --
> 2.7.4
>
On Fri, Apr 24, 2020 at 02:22:41PM +0800, Wanpeng Li wrote:
> From: Wanpeng Li <[email protected]>
>
> Introduce need_cancel_enter_guest() helper, we need to check some
> conditions before doing CONT_RUN, in addition, it can also catch
> the case vmexit occurred while another event was being delivered
> to guest software since vmx_complete_interrupts() adds the request
> bit.
>
> Tested-by: Haiwei Li <[email protected]>
> Cc: Haiwei Li <[email protected]>
> Signed-off-by: Wanpeng Li <[email protected]>
> ---
> arch/x86/kvm/vmx/vmx.c | 12 +++++++-----
> arch/x86/kvm/x86.c | 10 ++++++++--
> arch/x86/kvm/x86.h | 1 +
> 3 files changed, 16 insertions(+), 7 deletions(-)
>
> diff --git a/arch/x86/kvm/vmx/vmx.c b/arch/x86/kvm/vmx/vmx.c
> index f1f6638..5c21027 100644
> --- a/arch/x86/kvm/vmx/vmx.c
> +++ b/arch/x86/kvm/vmx/vmx.c
> @@ -6577,7 +6577,7 @@ 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)
> {
> - enum exit_fastpath_completion exit_fastpath;
> + enum exit_fastpath_completion exit_fastpath = EXIT_FASTPATH_NONE;
> struct vcpu_vmx *vmx = to_vmx(vcpu);
> unsigned long cr3, cr4;
>
> @@ -6754,10 +6754,12 @@ static enum exit_fastpath_completion vmx_vcpu_run(struct kvm_vcpu *vcpu)
> vmx_recover_nmi_blocking(vmx);
> vmx_complete_interrupts(vmx);
>
> - exit_fastpath = vmx_exit_handlers_fastpath(vcpu);
> - /* static call is better with retpolines */
> - if (exit_fastpath == EXIT_FASTPATH_CONT_RUN)
> - goto cont_run;
> + if (!kvm_need_cancel_enter_guest(vcpu)) {
> + exit_fastpath = vmx_exit_handlers_fastpath(vcpu);
> + /* static call is better with retpolines */
> + if (exit_fastpath == EXIT_FASTPATH_CONT_RUN)
> + goto cont_run;
> + }
>
> return exit_fastpath;
> }
> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> index 59958ce..4561104 100644
> --- a/arch/x86/kvm/x86.c
> +++ b/arch/x86/kvm/x86.c
> @@ -1581,6 +1581,13 @@ int kvm_emulate_wrmsr(struct kvm_vcpu *vcpu)
> }
> EXPORT_SYMBOL_GPL(kvm_emulate_wrmsr);
>
> +bool kvm_need_cancel_enter_guest(struct kvm_vcpu *vcpu)
What about kvm_vcpu_<???>_pending()? Not sure what a good ??? would be.
The "cancel_enter_guest" wording is a bit confusing when this is called
from the VM-Exit path.
> +{
> + return (vcpu->mode == EXITING_GUEST_MODE || kvm_request_pending(vcpu)
> + || need_resched() || signal_pending(current));
Parantheses around the whole statement are unnecessary. Personal preference
is to put the || before the newline.
> +}
> +EXPORT_SYMBOL_GPL(kvm_need_cancel_enter_guest);
> +
> /*
> * The fast path for frequent and performance sensitive wrmsr emulation,
> * i.e. the sending of IPI, sending IPI early in the VM-Exit flow reduces
> @@ -8373,8 +8380,7 @@ static int vcpu_enter_guest(struct kvm_vcpu *vcpu)
> if (kvm_lapic_enabled(vcpu) && vcpu->arch.apicv_active)
> kvm_x86_ops.sync_pir_to_irr(vcpu);
>
> - if (vcpu->mode == EXITING_GUEST_MODE || kvm_request_pending(vcpu)
> - || need_resched() || signal_pending(current)) {
> + if (kvm_need_cancel_enter_guest(vcpu)) {
> vcpu->mode = OUTSIDE_GUEST_MODE;
> smp_wmb();
> local_irq_enable();
> diff --git a/arch/x86/kvm/x86.h b/arch/x86/kvm/x86.h
> index 7b5ed8e..1906e7e 100644
> --- a/arch/x86/kvm/x86.h
> +++ b/arch/x86/kvm/x86.h
> @@ -364,5 +364,6 @@ static inline bool kvm_dr7_valid(u64 data)
> void kvm_load_guest_xsave_state(struct kvm_vcpu *vcpu);
> void kvm_load_host_xsave_state(struct kvm_vcpu *vcpu);
> u64 kvm_spec_ctrl_valid_bits(struct kvm_vcpu *vcpu);
> +bool kvm_need_cancel_enter_guest(struct kvm_vcpu *vcpu);
>
> #endif
> --
> 2.7.4
>
On Fri, Apr 24, 2020 at 02:22:42PM +0800, Wanpeng Li wrote:
> From: Wanpeng Li <[email protected]>
>
> Optimizing posted-interrupt delivery especially for the timer fastpath
> scenario, I observe kvm_x86_ops.deliver_posted_interrupt() has more latency
> then vmx_sync_pir_to_irr() in the case of timer fastpath scenario, since
> it needs to wait vmentry, after that it can handle external interrupt, ack
> the notification vector, read posted-interrupt descriptor etc, it is slower
> than evaluate and delivery during vmentry immediately approach. Let's skip
> sending interrupt to notify target pCPU and replace by vmx_sync_pir_to_irr()
> before each cont_run.
>
> Tested-by: Haiwei Li <[email protected]>
> Cc: Haiwei Li <[email protected]>
> Signed-off-by: Wanpeng Li <[email protected]>
> ---
> arch/x86/kvm/vmx/vmx.c | 9 ++++++---
> virt/kvm/kvm_main.c | 1 +
> 2 files changed, 7 insertions(+), 3 deletions(-)
>
> diff --git a/arch/x86/kvm/vmx/vmx.c b/arch/x86/kvm/vmx/vmx.c
> index 5c21027..d21b66b 100644
> --- a/arch/x86/kvm/vmx/vmx.c
> +++ b/arch/x86/kvm/vmx/vmx.c
> @@ -3909,8 +3909,9 @@ static int vmx_deliver_posted_interrupt(struct kvm_vcpu *vcpu, int vector)
> if (pi_test_and_set_on(&vmx->pi_desc))
> return 0;
>
> - if (!kvm_vcpu_trigger_posted_interrupt(vcpu, false))
> - kvm_vcpu_kick(vcpu);
> + if (vcpu != kvm_get_running_vcpu() &&
> + !kvm_vcpu_trigger_posted_interrupt(vcpu, false))
Bad indentation.
> + kvm_vcpu_kick(vcpu);
>
> return 0;
> }
> @@ -6757,8 +6758,10 @@ static enum exit_fastpath_completion vmx_vcpu_run(struct kvm_vcpu *vcpu)
> if (!kvm_need_cancel_enter_guest(vcpu)) {
> exit_fastpath = vmx_exit_handlers_fastpath(vcpu);
> /* static call is better with retpolines */
> - if (exit_fastpath == EXIT_FASTPATH_CONT_RUN)
> + if (exit_fastpath == EXIT_FASTPATH_CONT_RUN) {
> + vmx_sync_pir_to_irr(vcpu);
> goto cont_run;
> + }
> }
>
> return exit_fastpath;
> diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
> index e7436d0..6a289d1 100644
> --- a/virt/kvm/kvm_main.c
> +++ b/virt/kvm/kvm_main.c
> @@ -4633,6 +4633,7 @@ struct kvm_vcpu *kvm_get_running_vcpu(void)
>
> return vcpu;
> }
> +EXPORT_SYMBOL_GPL(kvm_get_running_vcpu);
>
> /**
> * kvm_get_running_vcpus - get the per-CPU array of currently running vcpus.
> --
> 2.7.4
>
On Sun, Apr 26, 2020 at 10:05:00AM +0800, Wanpeng Li wrote:
> On Fri, 24 Apr 2020 at 14:23, Wanpeng Li <[email protected]> wrote:
> >
> > From: Wanpeng Li <[email protected]>
> >
> > Introduce need_cancel_enter_guest() helper, we need to check some
> > conditions before doing CONT_RUN, in addition, it can also catch
> > the case vmexit occurred while another event was being delivered
> > to guest software since vmx_complete_interrupts() adds the request
> > bit.
> >
> > Tested-by: Haiwei Li <[email protected]>
> > Cc: Haiwei Li <[email protected]>
> > Signed-off-by: Wanpeng Li <[email protected]>
> > ---
> > arch/x86/kvm/vmx/vmx.c | 12 +++++++-----
> > arch/x86/kvm/x86.c | 10 ++++++++--
> > arch/x86/kvm/x86.h | 1 +
> > 3 files changed, 16 insertions(+), 7 deletions(-)
> >
> > diff --git a/arch/x86/kvm/vmx/vmx.c b/arch/x86/kvm/vmx/vmx.c
> > index f1f6638..5c21027 100644
> > --- a/arch/x86/kvm/vmx/vmx.c
> > +++ b/arch/x86/kvm/vmx/vmx.c
> > @@ -6577,7 +6577,7 @@ 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)
> > {
> > - enum exit_fastpath_completion exit_fastpath;
> > + enum exit_fastpath_completion exit_fastpath = EXIT_FASTPATH_NONE;
> > struct vcpu_vmx *vmx = to_vmx(vcpu);
> > unsigned long cr3, cr4;
> >
> > @@ -6754,10 +6754,12 @@ static enum exit_fastpath_completion vmx_vcpu_run(struct kvm_vcpu *vcpu)
> > vmx_recover_nmi_blocking(vmx);
> > vmx_complete_interrupts(vmx);
> >
> > - exit_fastpath = vmx_exit_handlers_fastpath(vcpu);
> > - /* static call is better with retpolines */
> > - if (exit_fastpath == EXIT_FASTPATH_CONT_RUN)
> > - goto cont_run;
> > + if (!kvm_need_cancel_enter_guest(vcpu)) {
> > + exit_fastpath = vmx_exit_handlers_fastpath(vcpu);
> > + /* static call is better with retpolines */
> > + if (exit_fastpath == EXIT_FASTPATH_CONT_RUN)
> > + goto cont_run;
> > + }
>
> The kvm_need_cancel_enter_guest() should not before
> vmx_exit_handlers_fastpath() which will break IPI fastpath. How about
> applying something like below, otherwise, maybe introduce another
> EXIT_FASTPATH_CONT_FAIL to indicate fails due to
> kvm_need_cancel_enter_guest() if checking it after
> vmx_exit_handlers_fastpath(), then we return 1 in vmx_handle_exit()
> directly instead of kvm_skip_emulated_instruction(). VMX-preemption
> timer exit doesn't need to skip emulated instruction but wrmsr
> TSCDEADLINE MSR exit does which results in a little complex here.
>
> Paolo, what do you think?
>
> diff --git a/arch/x86/kvm/vmx/vmx.c b/arch/x86/kvm/vmx/vmx.c
> index 853d3af..9317924 100644
> --- a/arch/x86/kvm/vmx/vmx.c
> +++ b/arch/x86/kvm/vmx/vmx.c
> @@ -6564,6 +6564,9 @@ static enum exit_fastpath_completion
> handle_fastpath_preemption_timer(struct kvm
> {
> struct vcpu_vmx *vmx = to_vmx(vcpu);
>
> + if (kvm_need_cancel_enter_guest(vcpu))
> + return EXIT_FASTPATH_NONE;
> +
> if (!vmx->req_immediate_exit &&
> !unlikely(vmx->loaded_vmcs->hv_timer_soft_disabled)) {
> kvm_lapic_expired_hv_timer(vcpu);
> @@ -6771,12 +6774,10 @@ static enum exit_fastpath_completion
> vmx_vcpu_run(struct kvm_vcpu *vcpu)
> vmx_recover_nmi_blocking(vmx);
> vmx_complete_interrupts(vmx);
>
> - if (!(kvm_need_cancel_enter_guest(vcpu))) {
> - exit_fastpath = vmx_exit_handlers_fastpath(vcpu);
> - if (exit_fastpath == EXIT_FASTPATH_CONT_RUN) {
> - vmx_sync_pir_to_irr(vcpu);
> - goto cont_run;
> - }
> + exit_fastpath = vmx_exit_handlers_fastpath(vcpu);
> + if (exit_fastpath == EXIT_FASTPATH_CONT_RUN) {
Relying on the handlers to check kvm_need_cancel_enter_guest() will be
error prone and costly to maintain. I also don't like that it buries the
logic.
What about adding another flavor, e.g.:
exit_fastpath = vmx_exit_handlers_fastpath(vcpu);
if (exit_fastpath == EXIT_FASTPATH_CONT_RUN &&
kvm_need_cancel_enter_guest(vcpu))
exit_fastpath = EXIT_FASTPATH_NOP;
That would also allow you to enable preemption timer without first having
to add CONT_RUN, which would be a very good thing for bisection.
> + vmx_sync_pir_to_irr(vcpu);
> + goto cont_run;
> }
>
> return exit_fastpath;
> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> index 99061ba..11b309c 100644
> --- a/arch/x86/kvm/x86.c
> +++ b/arch/x86/kvm/x86.c
> @@ -1618,6 +1618,9 @@ static int
> handle_fastpath_set_x2apic_icr_irqoff(struct kvm_vcpu *vcpu, u64 data
>
> static int handle_fastpath_set_tscdeadline(struct kvm_vcpu *vcpu, u64 data)
> {
> + if (kvm_need_cancel_enter_guest(vcpu))
> + return 1;
> +
> if (!kvm_x86_ops.set_hv_timer ||
> kvm_mwait_in_guest(vcpu->kvm) ||
> kvm_can_post_timer_interrupt(vcpu))
On Fri, Apr 24, 2020 at 02:22:43PM +0800, Wanpeng Li wrote:
> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> index 4561104..99061ba 100644
> --- a/arch/x86/kvm/x86.c
> +++ b/arch/x86/kvm/x86.c
> @@ -1616,27 +1616,45 @@ 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)
> +{
> + if (!kvm_x86_ops.set_hv_timer ||
> + kvm_mwait_in_guest(vcpu->kvm) ||
> + kvm_can_post_timer_interrupt(vcpu))
Bad indentation.
> + return 1;
> +
> + kvm_set_lapic_tscdeadline_msr(vcpu, data);
> + return 0;
> +}
> +
> 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:
> + 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
>
On Fri, Apr 24, 2020 at 02:22:44PM +0800, Wanpeng Li wrote:
> From: Wanpeng Li <[email protected]>
>
> 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):
>
> 5540.5ns -> 4602ns 17%
>
> kvm-unit-test/vmexit.flat:
>
> w/o avanced timer:
> tscdeadline_immed: 2885 -> 2431.25 15.7%
> tscdeadline: 5668.75 -> 5188.5 8.4%
>
> w/ adaptive advance timer default -1:
> tscdeadline_immed: 2965.25 -> 2520 15.0%
> tscdeadline: 4663.75 -> 4537 2.7%
>
> Tested-by: Haiwei Li <[email protected]>
> Cc: Haiwei Li <[email protected]>
> Signed-off-by: Wanpeng Li <[email protected]>
> ---
> arch/x86/kvm/vmx/vmx.c | 16 ++++++++++++++++
> 1 file changed, 16 insertions(+)
>
> diff --git a/arch/x86/kvm/vmx/vmx.c b/arch/x86/kvm/vmx/vmx.c
> index d21b66b..028967a 100644
> --- a/arch/x86/kvm/vmx/vmx.c
> +++ b/arch/x86/kvm/vmx/vmx.c
> @@ -6560,12 +6560,28 @@ void vmx_update_host_rsp(struct vcpu_vmx *vmx, unsigned long host_rsp)
> }
> }
>
> +static enum exit_fastpath_completion handle_fastpath_preemption_timer(struct kvm_vcpu *vcpu)
Somewhat offtopic, would it make sense to add a fastpath_t typedef? These
enum lines are a bit long...
> +{
> + struct vcpu_vmx *vmx = to_vmx(vcpu);
> +
> + if (!vmx->req_immediate_exit &&
> + !unlikely(vmx->loaded_vmcs->hv_timer_soft_disabled)) {
Bad indentation.
Also, this is is identical to handle_preemption_timer(), why not something
like:
static bool __handle_preemption_timer(struct vcpu)
{
struct vcpu_vmx *vmx = to_vmx(vcpu);
if (!vmx->req_immediate_exit &&
!unlikely(vmx->loaded_vmcs->hv_timer_soft_disabled)) {
kvm_lapic_expired_hv_timer(vcpu);
return true;
}
return false;
}
static enum exit_fastpath_completion handle_fastpath_preemption_timer(struct kvm_vcpu *vcpu)
{
if (__handle_preemption_timer(vcpu))
return EXIT_FASTPATH_CONT_RUN;
return EXIT_FASTPATH_NONE;
}
static int handle_preemption_timer(struct kvm_vcpu *vcpu)
{
__handle_preemption_timer(vcpu);
return 1;
}
> + kvm_lapic_expired_hv_timer(vcpu);
> + trace_kvm_exit(EXIT_REASON_PREEMPTION_TIMER, vcpu, KVM_ISA_VMX);
> + 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) && vcpu->arch.apicv_active) {
> 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
>
On Tue, 28 Apr 2020 at 02:42, Sean Christopherson
<[email protected]> wrote:
>
> On Fri, Apr 24, 2020 at 02:22:44PM +0800, Wanpeng Li wrote:
> > From: Wanpeng Li <[email protected]>
> >
> > 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):
> >
> > 5540.5ns -> 4602ns 17%
> >
> > kvm-unit-test/vmexit.flat:
> >
> > w/o avanced timer:
> > tscdeadline_immed: 2885 -> 2431.25 15.7%
> > tscdeadline: 5668.75 -> 5188.5 8.4%
> >
> > w/ adaptive advance timer default -1:
> > tscdeadline_immed: 2965.25 -> 2520 15.0%
> > tscdeadline: 4663.75 -> 4537 2.7%
> >
> > Tested-by: Haiwei Li <[email protected]>
> > Cc: Haiwei Li <[email protected]>
> > Signed-off-by: Wanpeng Li <[email protected]>
> > ---
> > arch/x86/kvm/vmx/vmx.c | 16 ++++++++++++++++
> > 1 file changed, 16 insertions(+)
> >
> > diff --git a/arch/x86/kvm/vmx/vmx.c b/arch/x86/kvm/vmx/vmx.c
> > index d21b66b..028967a 100644
> > --- a/arch/x86/kvm/vmx/vmx.c
> > +++ b/arch/x86/kvm/vmx/vmx.c
> > @@ -6560,12 +6560,28 @@ void vmx_update_host_rsp(struct vcpu_vmx *vmx, unsigned long host_rsp)
> > }
> > }
> >
> > +static enum exit_fastpath_completion handle_fastpath_preemption_timer(struct kvm_vcpu *vcpu)
>
> Somewhat offtopic, would it make sense to add a fastpath_t typedef? These
> enum lines are a bit long...
>
> > +{
> > + struct vcpu_vmx *vmx = to_vmx(vcpu);
> > +
> > + if (!vmx->req_immediate_exit &&
> > + !unlikely(vmx->loaded_vmcs->hv_timer_soft_disabled)) {
>
> Bad indentation.
>
> Also, this is is identical to handle_preemption_timer(), why not something
> like:
>
> static bool __handle_preemption_timer(struct vcpu)
> {
> struct vcpu_vmx *vmx = to_vmx(vcpu);
>
> if (!vmx->req_immediate_exit &&
> !unlikely(vmx->loaded_vmcs->hv_timer_soft_disabled)) {
> kvm_lapic_expired_hv_timer(vcpu);
> return true;
> }
>
> return false;
> }
>
> static enum exit_fastpath_completion handle_fastpath_preemption_timer(struct kvm_vcpu *vcpu)
> {
> if (__handle_preemption_timer(vcpu))
> return EXIT_FASTPATH_CONT_RUN;
> return EXIT_FASTPATH_NONE;
> }
>
> static int handle_preemption_timer(struct kvm_vcpu *vcpu)
> {
> __handle_preemption_timer(vcpu);
> return 1;
> }
>
Great! Thanks for making this nicer.
Wanpeng
On Tue, 28 Apr 2020 at 02:36, Sean Christopherson
<[email protected]> wrote:
>
> On Sun, Apr 26, 2020 at 10:05:00AM +0800, Wanpeng Li wrote:
> > On Fri, 24 Apr 2020 at 14:23, Wanpeng Li <[email protected]> wrote:
> > >
> > > From: Wanpeng Li <[email protected]>
> > >
> > > Introduce need_cancel_enter_guest() helper, we need to check some
> > > conditions before doing CONT_RUN, in addition, it can also catch
> > > the case vmexit occurred while another event was being delivered
> > > to guest software since vmx_complete_interrupts() adds the request
> > > bit.
> > >
> > > Tested-by: Haiwei Li <[email protected]>
> > > Cc: Haiwei Li <[email protected]>
> > > Signed-off-by: Wanpeng Li <[email protected]>
> > > ---
> > > arch/x86/kvm/vmx/vmx.c | 12 +++++++-----
> > > arch/x86/kvm/x86.c | 10 ++++++++--
> > > arch/x86/kvm/x86.h | 1 +
> > > 3 files changed, 16 insertions(+), 7 deletions(-)
> > >
> > > diff --git a/arch/x86/kvm/vmx/vmx.c b/arch/x86/kvm/vmx/vmx.c
> > > index f1f6638..5c21027 100644
> > > --- a/arch/x86/kvm/vmx/vmx.c
> > > +++ b/arch/x86/kvm/vmx/vmx.c
> > > @@ -6577,7 +6577,7 @@ 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)
> > > {
> > > - enum exit_fastpath_completion exit_fastpath;
> > > + enum exit_fastpath_completion exit_fastpath = EXIT_FASTPATH_NONE;
> > > struct vcpu_vmx *vmx = to_vmx(vcpu);
> > > unsigned long cr3, cr4;
> > >
> > > @@ -6754,10 +6754,12 @@ static enum exit_fastpath_completion vmx_vcpu_run(struct kvm_vcpu *vcpu)
> > > vmx_recover_nmi_blocking(vmx);
> > > vmx_complete_interrupts(vmx);
> > >
> > > - exit_fastpath = vmx_exit_handlers_fastpath(vcpu);
> > > - /* static call is better with retpolines */
> > > - if (exit_fastpath == EXIT_FASTPATH_CONT_RUN)
> > > - goto cont_run;
> > > + if (!kvm_need_cancel_enter_guest(vcpu)) {
> > > + exit_fastpath = vmx_exit_handlers_fastpath(vcpu);
> > > + /* static call is better with retpolines */
> > > + if (exit_fastpath == EXIT_FASTPATH_CONT_RUN)
> > > + goto cont_run;
> > > + }
> >
> > The kvm_need_cancel_enter_guest() should not before
> > vmx_exit_handlers_fastpath() which will break IPI fastpath. How about
> > applying something like below, otherwise, maybe introduce another
> > EXIT_FASTPATH_CONT_FAIL to indicate fails due to
> > kvm_need_cancel_enter_guest() if checking it after
> > vmx_exit_handlers_fastpath(), then we return 1 in vmx_handle_exit()
> > directly instead of kvm_skip_emulated_instruction(). VMX-preemption
> > timer exit doesn't need to skip emulated instruction but wrmsr
> > TSCDEADLINE MSR exit does which results in a little complex here.
> >
> > Paolo, what do you think?
> >
> > diff --git a/arch/x86/kvm/vmx/vmx.c b/arch/x86/kvm/vmx/vmx.c
> > index 853d3af..9317924 100644
> > --- a/arch/x86/kvm/vmx/vmx.c
> > +++ b/arch/x86/kvm/vmx/vmx.c
> > @@ -6564,6 +6564,9 @@ static enum exit_fastpath_completion
> > handle_fastpath_preemption_timer(struct kvm
> > {
> > struct vcpu_vmx *vmx = to_vmx(vcpu);
> >
> > + if (kvm_need_cancel_enter_guest(vcpu))
> > + return EXIT_FASTPATH_NONE;
> > +
> > if (!vmx->req_immediate_exit &&
> > !unlikely(vmx->loaded_vmcs->hv_timer_soft_disabled)) {
> > kvm_lapic_expired_hv_timer(vcpu);
> > @@ -6771,12 +6774,10 @@ static enum exit_fastpath_completion
> > vmx_vcpu_run(struct kvm_vcpu *vcpu)
> > vmx_recover_nmi_blocking(vmx);
> > vmx_complete_interrupts(vmx);
> >
> > - if (!(kvm_need_cancel_enter_guest(vcpu))) {
> > - exit_fastpath = vmx_exit_handlers_fastpath(vcpu);
> > - if (exit_fastpath == EXIT_FASTPATH_CONT_RUN) {
> > - vmx_sync_pir_to_irr(vcpu);
> > - goto cont_run;
> > - }
> > + exit_fastpath = vmx_exit_handlers_fastpath(vcpu);
> > + if (exit_fastpath == EXIT_FASTPATH_CONT_RUN) {
>
> Relying on the handlers to check kvm_need_cancel_enter_guest() will be
> error prone and costly to maintain. I also don't like that it buries the
> logic.
>
> What about adding another flavor, e.g.:
>
> exit_fastpath = vmx_exit_handlers_fastpath(vcpu);
> if (exit_fastpath == EXIT_FASTPATH_CONT_RUN &&
> kvm_need_cancel_enter_guest(vcpu))
> exit_fastpath = EXIT_FASTPATH_NOP;
>
> That would also allow you to enable preemption timer without first having
> to add CONT_RUN, which would be a very good thing for bisection.
I miss understand the second part, do you mean don't need to add
CONT_RUN in patch 1/5?
Wanpeng
On Tue, 28 Apr 2020 at 02:26, Sean Christopherson
<[email protected]> wrote:
>
> On Fri, Apr 24, 2020 at 02:22:40PM +0800, Wanpeng Li wrote:
> > From: Wanpeng Li <[email protected]>
> >
> > Introduce generic fastpath handler to handle MSR fastpath, VMX-preemption
> > timer fastpath etc. In addition, we can't observe benefit from single
> > target IPI fastpath when APICv is disabled, let's just enable IPI and
> > Timer fastpath when APICv is enabled for now.
>
> There are three different changes being squished into a single patch:
>
> - Refactor code to add helper
> - Change !APICv behavior for WRMSR fastpath
> - Introduce EXIT_FASTPATH_CONT_RUN
>
> I don't think you necessarily need to break this into three separate
> patches, but's the !APICv change needs to be a standalone patch, especially
> given the shortlog. E.g. the refactoring could be introduced along with
> the second fastpath case, and CONT_RUN could be introduced with its first
> usage.
Agreed, will split to two separate patches.
Wanpeng
On Tue, Apr 28, 2020 at 08:44:13AM +0800, Wanpeng Li wrote:
> On Tue, 28 Apr 2020 at 02:36, Sean Christopherson
> <[email protected]> wrote:
> >
> > > @@ -6771,12 +6774,10 @@ static enum exit_fastpath_completion
> > > vmx_vcpu_run(struct kvm_vcpu *vcpu)
> > > vmx_recover_nmi_blocking(vmx);
> > > vmx_complete_interrupts(vmx);
> > >
> > > - if (!(kvm_need_cancel_enter_guest(vcpu))) {
> > > - exit_fastpath = vmx_exit_handlers_fastpath(vcpu);
> > > - if (exit_fastpath == EXIT_FASTPATH_CONT_RUN) {
> > > - vmx_sync_pir_to_irr(vcpu);
> > > - goto cont_run;
> > > - }
> > > + exit_fastpath = vmx_exit_handlers_fastpath(vcpu);
> > > + if (exit_fastpath == EXIT_FASTPATH_CONT_RUN) {
> >
> > Relying on the handlers to check kvm_need_cancel_enter_guest() will be
> > error prone and costly to maintain. I also don't like that it buries the
> > logic.
> >
> > What about adding another flavor, e.g.:
> >
> > exit_fastpath = vmx_exit_handlers_fastpath(vcpu);
> > if (exit_fastpath == EXIT_FASTPATH_CONT_RUN &&
> > kvm_need_cancel_enter_guest(vcpu))
> > exit_fastpath = EXIT_FASTPATH_NOP;
> >
> > That would also allow you to enable preemption timer without first having
> > to add CONT_RUN, which would be a very good thing for bisection.
>
> I miss understand the second part, do you mean don't need to add
> CONT_RUN in patch 1/5?
Yes, with the disclaimer that I haven't worked through all the flows to
ensure it's actually doable and/or a good idea.
The idea is to add EXIT_FASTPATH_NOP and use that for the preemption timer
fastpath. KVM would still go through it's full run loop, but it would skip
invoking the exit handler. In theory that would disassociate fast handling
of the preemption timer from resuming the guest without going back to the
run loop, i.e. provide a separate bisection point for enabling CONT_RUN.
Like I said, might not be a good idea, e.g. if preemption timer ends up
being the only user of EXIT_FASTPATH_CONT_RUN then EXIT_FASTPATH_NOP is a
waste of space.
Side topic, what about EXIT_FASTPATH_RESUME instead of CONT_RUN? Or maybe
REENTER_GUEST? Something that start with RE :-)
On Tue, 28 Apr 2020 at 02:30, Sean Christopherson
<[email protected]> wrote:
>
> On Fri, Apr 24, 2020 at 02:22:41PM +0800, Wanpeng Li wrote:
> > From: Wanpeng Li <[email protected]>
> >
> > Introduce need_cancel_enter_guest() helper, we need to check some
> > conditions before doing CONT_RUN, in addition, it can also catch
> > the case vmexit occurred while another event was being delivered
> > to guest software since vmx_complete_interrupts() adds the request
> > bit.
> >
> > Tested-by: Haiwei Li <[email protected]>
> > Cc: Haiwei Li <[email protected]>
> > Signed-off-by: Wanpeng Li <[email protected]>
> > ---
> > arch/x86/kvm/vmx/vmx.c | 12 +++++++-----
> > arch/x86/kvm/x86.c | 10 ++++++++--
> > arch/x86/kvm/x86.h | 1 +
> > 3 files changed, 16 insertions(+), 7 deletions(-)
> >
> > diff --git a/arch/x86/kvm/vmx/vmx.c b/arch/x86/kvm/vmx/vmx.c
> > index f1f6638..5c21027 100644
> > --- a/arch/x86/kvm/vmx/vmx.c
> > +++ b/arch/x86/kvm/vmx/vmx.c
> > @@ -6577,7 +6577,7 @@ 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)
> > {
> > - enum exit_fastpath_completion exit_fastpath;
> > + enum exit_fastpath_completion exit_fastpath = EXIT_FASTPATH_NONE;
> > struct vcpu_vmx *vmx = to_vmx(vcpu);
> > unsigned long cr3, cr4;
> >
> > @@ -6754,10 +6754,12 @@ static enum exit_fastpath_completion vmx_vcpu_run(struct kvm_vcpu *vcpu)
> > vmx_recover_nmi_blocking(vmx);
> > vmx_complete_interrupts(vmx);
> >
> > - exit_fastpath = vmx_exit_handlers_fastpath(vcpu);
> > - /* static call is better with retpolines */
> > - if (exit_fastpath == EXIT_FASTPATH_CONT_RUN)
> > - goto cont_run;
> > + if (!kvm_need_cancel_enter_guest(vcpu)) {
> > + exit_fastpath = vmx_exit_handlers_fastpath(vcpu);
> > + /* static call is better with retpolines */
> > + if (exit_fastpath == EXIT_FASTPATH_CONT_RUN)
> > + goto cont_run;
> > + }
> >
> > return exit_fastpath;
> > }
> > diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> > index 59958ce..4561104 100644
> > --- a/arch/x86/kvm/x86.c
> > +++ b/arch/x86/kvm/x86.c
> > @@ -1581,6 +1581,13 @@ int kvm_emulate_wrmsr(struct kvm_vcpu *vcpu)
> > }
> > EXPORT_SYMBOL_GPL(kvm_emulate_wrmsr);
> >
> > +bool kvm_need_cancel_enter_guest(struct kvm_vcpu *vcpu)
>
> What about kvm_vcpu_<???>_pending()? Not sure what a good ??? would be.
> The "cancel_enter_guest" wording is a bit confusing when this is called
> from the VM-Exit path.
>
> > +{
> > + return (vcpu->mode == EXITING_GUEST_MODE || kvm_request_pending(vcpu)
> > + || need_resched() || signal_pending(current));
>
> Parantheses around the whole statement are unnecessary. Personal preference
> is to put the || before the newline.
Handle the comments in v4.
Wanpeng