kvm: nVMX: Pending debug exceptions trump expired VMX-preemption timer
PATCH 1/2 kvm: nVMX: Pending debug exceptions trump expired VMX-preemption timer
Previously, if L1 launched vmcs12 with both pending debug exceptions and an already-expired VMX-preemption timer, the pending debug exceptions were lost due to a priority inversion between a pending #DB exception and a "VMX-preemption timer expired" VM-exit from L2 to L1.
In this scenario, L0 constructs a vmcs02 that has both a zero-valued VMX-preemption timer (assuming enable_preemption_timer is set) and pending debug exceptions. When the vmcs02 is launched/resumed, the hardware correctly prioritizes the pending debug exceptions. At this point, L0 intercepts the resulting #DB trap and queues it up for redelivery. However, when checking for nested events in software, L0 incorrectly prioritizes the "VMX-preemption timer expired" VM-exit from L2 to L1.
Technically, nested events should probably be blocked at this point. Hardware has already determined that the #DB trap is the next event that should happen. L0 just got in the way because it was concerned about infinite IDT vectoring loops.
Logically, the enqueued #DB trap is quite similar to a "reinjected" event resulting from interrupted IDT-vectoring. Treating it as such fixes the problem, since nested events are blocked when a reinjected event is present. However, there are some ways in which the enqueued interrupted IDT-vectoring. In particular, it should not be recorded in the IDT-vectoring information field of the vmcs12 in the event of a synthesized VM-exit from L2 to L1. I don't believe that path should ever be taken, since the #DB trap should take priority over any synthesized VM-exit from L2 to L1.
Recategorize both the reinjected #DB and #AC exceptions as "reinjected" exceptions. For consistency, do the same thing for SVM, even though it doesn't have a VMX-preemption timer equivalent.
--- arch/x86/kvm/svm/svm.c | 4 ++-- arch/x86/kvm/vmx/vmx.c | 4 ++-- 2 files changed, 4 insertions(+), 4 deletions(-)
arch/x86/kvm/svm/svm.c +2 -2
@@ -1739,7 +1739,7 @@ static int db_interception(struct vcpu_svm *svm)if (!(svm->vcpu.guest_debug & (KVM_GUESTDBG_SINGLESTEP |KVM_GUESTDBG_USE_HW_BP )) && !svm->nmi_singlestep) { -kvm_queue_exception (&svm->vcpu,DB_VECTOR ); +kvm_requeue_exception (&svm->vcpu,DB_VECTOR );return 1 ; }
@@ -1778,7 +1778,7 @@ static int ud_interception(struct vcpu_svm *svm)static int ac_interception (struct vcpu_svm *svm) { -kvm_queue_exception_e (&svm->vcpu,AC_VECTOR ,0 ); +kvm_requeue_exception_e (&svm->vcpu,AC_VECTOR ,0 );return 1 ; }
arch/x86/kvm/vmx/vmx.c +2 -2
@@ -4682,7 +4682,7 @@ static int handle_exception_nmi(struct kvm_vcpu *vcpu)if (is_icebp (intr_info))WARN_ON (!skip_emulated_instruction (vcpu)); -kvm_queue_exception (vcpu,DB_VECTOR ); +kvm_requeue_exception (vcpu,DB_VECTOR );return 1 ; } kvm_run->debug.arch.dr6 = dr6 |DR6_FIXED_1 ;
@@ -4703,7 +4703,7 @@ static int handle_exception_nmi(struct kvm_vcpu *vcpu)break ;case AC_VECTOR :if (guest_inject_ac (vcpu)) { -kvm_queue_exception_e (vcpu,AC_VECTOR , error_code); +kvm_requeue_exception_e (vcpu,AC_VECTOR , error_code);return 1 ; }
Signature · 1 line
2.26.0.110.g2183baf09c-goog
PATCH 2/2 kvm: nVMX: Single-step traps trump expired VMX-preemption timer
Previously, if the hrtimer for the nested VMX-preemption timer fired while L0 was emulating an L2 instruction with RFLAGS.TF set, the synthesized single-step trap would be unceremoniously dropped when synthesizing the "VMX-preemption timer expired" VM-exit from L2 to L1.
To fix this, don't synthesize a "VMX-preemption timer expired" VM-exit from L2 to L1 when there is a pending debug trap, such as a single-step trap.
--- arch/x86/kvm/vmx/nested.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-)
arch/x86/kvm/vmx/nested.c +3 -1
@@ -3690,7 +3690,9 @@ static int vmx_check_nested_events(struct kvm_vcpu *vcpu) vmx->nested.preemption_timer_expired) {if (block_nested_events)return -EBUSY ; -nested_vmx_vmexit (vcpu,EXIT_REASON_PREEMPTION_TIMER ,0 ,0 ); +if (!vmx_pending_dbg_trap (vcpu)) +nested_vmx_vmexit (vcpu,EXIT_REASON_PREEMPTION_TIMER , +0 ,0 );return 0 ; }
Signature · 1 line
2.26.0.110.g2183baf09c-goog
Re: [PATCH 2/2] kvm: nVMX: Single-step traps trump expired VMX-preemption timer
Quoted 22 lines · Jim Mattson
Previously, if the hrtimer for the nested VMX-preemption timer fired while L0 was emulating an L2 instruction with RFLAGS.TF set, the synthesized single-step trap would be unceremoniously dropped when synthesizing the "VMX-preemption timer expired" VM-exit from L2 to L1.
To fix this, don't synthesize a "VMX-preemption timer expired" VM-exit from L2 to L1 when there is a pending debug trap, such as a single-step trap.
Fixes f4124500c2c13 ("KVM: nVMX: Fully emulate preemption timer")Signed-off-by Jim Mattson <[email protected]>Reviewed-by Oliver Upton <[email protected]>Reviewed-by Peter Shier <[email protected]>--- arch/x86/kvm/vmx/nested.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-)arch/x86/kvm/vmx/nested.c +0
-0
arch/x86/kvm/vmx/nested.c static int vmx_check_nested_events(struct kvm_vcpu *vcpu) · line 3693
Quoted 5 lines
arch/x86/kvm/vmx/nested.c +1
-1@@ -3690,7 +3690,9 @@ static int vmx_check_nested_events(struct kvm_vcpu *vcpu) vmx->nested.preemption_timer_expired) {if (block_nested_events)return -EBUSY ; -nested_vmx_vmexit (vcpu,EXIT_REASON_PREEMPTION_TIMER ,0 ,0 ); +if (!vmx_pending_dbg_trap (vcpu))
IMO this one warrants a comment. It's not immediately obvious that this only applies to #DBs that are being injected into L2, and that returning -EBUSY will do the wrong thing.
arch/x86/kvm/vmx/nested.c static int vmx_check_nested_events(struct kvm_vcpu *vcpu) · line 3695
Quoted 2 lines
arch/x86/kvm/vmx/nested.c +2
-0+nested_vmx_vmexit (vcpu,EXIT_REASON_PREEMPTION_TIMER , +0 ,0 );
I'd just let the "0, 0);" poke out past 80 chars.
Quoted 6 lines
arch/x86/kvm/vmx/nested.c +0
-0return 0 ; }Signature · 1 line
2.26.0.110.g2183baf09c-goog
Re: [PATCH 2/2] kvm: nVMX: Single-step traps trump expired VMX-preemption timer
Quoted 19 lines · Sean Christopherson
Quoted 17 lines · Jim Mattson
Previously, if the hrtimer for the nested VMX-preemption timer fired while L0 was emulating an L2 instruction with RFLAGS.TF set, the synthesized single-step trap would be unceremoniously dropped when synthesizing the "VMX-preemption timer expired" VM-exit from L2 to L1.
To fix this, don't synthesize a "VMX-preemption timer expired" VM-exit from L2 to L1 when there is a pending debug trap, such as a single-step trap.
Fixes f4124500c2c13 ("KVM: nVMX: Fully emulate preemption timer")Signed-off-by Jim Mattson <[email protected]>Reviewed-by Oliver Upton <[email protected]>Reviewed-by Peter Shier <[email protected]>--- arch/x86/kvm/vmx/nested.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-)
Quoted 14 lines
arch/x86/kvm/vmx/nested.c static int vmx_check_nested_events(struct kvm_vcpu *vcpu) · line 3693
Quoted 10 lines
arch/x86/kvm/vmx/nested.c +1
-1@@ -3690,7 +3690,9 @@ static int vmx_check_nested_events(struct kvm_vcpu *vcpu) vmx->nested.preemption_timer_expired) {if (block_nested_events)return -EBUSY ; -nested_vmx_vmexit (vcpu,EXIT_REASON_PREEMPTION_TIMER ,0 ,0 ); +if (!vmx_pending_dbg_trap (vcpu))IMO this one warrants a comment. It's not immediately obvious that this only applies to #DBs that are being injected into L2, and that returning
-EBUSYwill do the wrong thing.
Regarding -EBUSY, I'm in complete agreement. However, I'm not sure what the potential confusion is regarding the event. Are you suggesting that one might think that we have a #DB to deliver to L1 while we're in guest mode? IIRC, that can happen under SVM, but I don't believe it can happen under VMX.
Quoted 4 lines
arch/x86/kvm/vmx/nested.c static int vmx_check_nested_events(struct kvm_vcpu *vcpu) · line 3695
Quoted 2 lines
arch/x86/kvm/vmx/nested.c +2
-0+nested_vmx_vmexit (vcpu,EXIT_REASON_PREEMPTION_TIMER , +0 ,0 );I'd just let the "0, 0);" poke out past 80 chars.
Oh, you rascal, you!
Quoted 6 lines
Quoted 6 lines
arch/x86/kvm/vmx/nested.c +0
-1return 0 ; } --2.26.0.110.g2183baf09c-goog
Re: [PATCH 2/2] kvm: nVMX: Single-step traps trump expired VMX-preemption timer
Quoted 36 lines · Jim Mattson
Quoted 19 lines · Sean Christopherson
Quoted 17 lines · Jim Mattson
Previously, if the hrtimer for the nested VMX-preemption timer fired while L0 was emulating an L2 instruction with RFLAGS.TF set, the synthesized single-step trap would be unceremoniously dropped when synthesizing the "VMX-preemption timer expired" VM-exit from L2 to L1.
To fix this, don't synthesize a "VMX-preemption timer expired" VM-exit from L2 to L1 when there is a pending debug trap, such as a single-step trap.
Fixes f4124500c2c13 ("KVM: nVMX: Fully emulate preemption timer")Signed-off-by Jim Mattson <[email protected]>Reviewed-by Oliver Upton <[email protected]>Reviewed-by Peter Shier <[email protected]>--- arch/x86/kvm/vmx/nested.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-)Quoted 14 lines
arch/x86/kvm/vmx/nested.c static int vmx_check_nested_events(struct kvm_vcpu *vcpu) · line 3693
Quoted 10 lines
arch/x86/kvm/vmx/nested.c +1
-1@@ -3690,7 +3690,9 @@ static int vmx_check_nested_events(struct kvm_vcpu *vcpu) vmx->nested.preemption_timer_expired) {if (block_nested_events)return -EBUSY ; -nested_vmx_vmexit (vcpu,EXIT_REASON_PREEMPTION_TIMER ,0 ,0 ); +if (!vmx_pending_dbg_trap (vcpu))IMO this one warrants a comment. It's not immediately obvious that this only applies to #DBs that are being injected into L2, and that returning
-EBUSYwill do the wrong thing.
Quoted 5 lines
Regarding
-EBUSY, I'm in complete agreement. However, I'm not sure what the potential confusion is regarding the event. Are you suggesting that one might think that we have a #DB to deliver to L1 while we're in guest mode? IIRC, that can happen under SVM, but I don't believe it can happen under VMX.
The potential confusion is that vcpu->arch.exception.pending was already checked, twice. It makes one wonder why it needs to be checked a third time. And actually, I think that's probably a good indicator that singling out single-step #DB isn't the correct fix, it just happens to be the only case that's been encountered thus far, e.g. a #PF when fetching the instr for emulation should also get priority over the preemption timer. On real hardware, expiration of the preemption timer while vectoring a #PF wouldn't wouldn't get recognized until the next instruction boundary, i.e. at the start of the first instruction of the #PF handler. Dropping the #PF isn't a problem in most cases, because unlike the single-step #DB, it will be re-encountered when L1 resumes L2. But, dropping the #PF is still wrong.
In general, interception of an event doesn't change the priority of events, e.g. INTR shouldn't get priority over NMI just because if L1 wants to intercept INTR but not NMI.
TL;DR: I think the fix should instead be:
arch/x86/kvm/vmx/nested.c +16 -7
@@ -3724,9 +3724,10 @@ static int vmx_check_nested_events(struct kvm_vcpu *vcpu)/* * Process any exceptions that are not debug traps before MTF. */ -if (vcpu->arch.exception.pending && - !vmx_pending_dbg_trap (vcpu) && -nested_vmx_check_exception (vcpu, &exit_qual)) { +if (vcpu->arch.exception.pending && !vmx_pending_dbg_trap (vcpu)) +if (!nested_vmx_check_exception (vcpu, &exit_qual)) +return 0 ; +if (block_nested_events)return -EBUSY ;nested_vmx_inject_exception_vmexit (vcpu, exit_qual);
@@ -3741,8 +3742,10 @@ static int vmx_check_nested_events(struct kvm_vcpu *vcpu)return 0 ; } -if (vcpu->arch.exception.pending && -nested_vmx_check_exception (vcpu, &exit_qual)) { +if (vcpu->arch.exception.pending) { +if (!nested_vmx_check_exception (vcpu, &exit_qual)) +return 0 ; +if (block_nested_events)return -EBUSY ;nested_vmx_inject_exception_vmexit (vcpu, exit_qual);
@@ -3757,7 +3760,10 @@ static int vmx_check_nested_events(struct kvm_vcpu *vcpu)return 0 ; } -if (vcpu->arch.nmi_pending &&nested_exit_on_nmi (vcpu)) { +if (vcpu->arch.nmi_pending) { +if (!nested_exit_on_nmi (vcpu)) +return 0 ; +if (block_nested_events)return -EBUSY ; nested_vmx_vmexit(vcpu, EXIT_REASON_EXCEPTION_NMI,
@@ -3772,7 +3778,10 @@ static int vmx_check_nested_events(struct kvm_vcpu *vcpu)return 0 ; } -if (kvm_cpu_has_interrupt (vcpu) &&nested_exit_on_intr (vcpu)) { + if (kvm_cpu_has_interrupt(vcpu) { +if (!nested_exit_on_intr (vcpu)) +return 0 ; +if (block_nested_events)return -EBUSY ;nested_vmx_vmexit (vcpu,EXIT_REASON_EXTERNAL_INTERRUPT ,0 ,0 );
Re: [PATCH 2/2] kvm: nVMX: Single-step traps trump expired VMX-preemption timer
Quoted 105 lines · Sean Christopherson
Quoted 36 lines · Jim Mattson
Quoted 19 lines · Sean Christopherson
Quoted 17 lines · Jim Mattson
Previously, if the hrtimer for the nested VMX-preemption timer fired while L0 was emulating an L2 instruction with RFLAGS.TF set, the synthesized single-step trap would be unceremoniously dropped when synthesizing the "VMX-preemption timer expired" VM-exit from L2 to L1.
To fix this, don't synthesize a "VMX-preemption timer expired" VM-exit from L2 to L1 when there is a pending debug trap, such as a single-step trap.
Fixes f4124500c2c13 ("KVM: nVMX: Fully emulate preemption timer")Signed-off-by Jim Mattson <[email protected]>Reviewed-by Oliver Upton <[email protected]>Reviewed-by Peter Shier <[email protected]>--- arch/x86/kvm/vmx/nested.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-)Quoted 14 lines
arch/x86/kvm/vmx/nested.c static int vmx_check_nested_events(struct kvm_vcpu *vcpu) · line 3693
Quoted 10 lines
arch/x86/kvm/vmx/nested.c +1
-1@@ -3690,7 +3690,9 @@ static int vmx_check_nested_events(struct kvm_vcpu *vcpu) vmx->nested.preemption_timer_expired) {if (block_nested_events)return -EBUSY ; -nested_vmx_vmexit (vcpu,EXIT_REASON_PREEMPTION_TIMER ,0 ,0 ); +if (!vmx_pending_dbg_trap (vcpu))IMO this one warrants a comment. It's not immediately obvious that this only applies to #DBs that are being injected into L2, and that returning
-EBUSYwill do the wrong thing.Quoted 5 lines
Regarding
-EBUSY, I'm in complete agreement. However, I'm not sure what the potential confusion is regarding the event. Are you suggesting that one might think that we have a #DB to deliver to L1 while we're in guest mode? IIRC, that can happen under SVM, but I don't believe it can happen under VMX.The potential confusion is that
vcpu->arch.exception.pendingwas already checked, twice. It makes one wonder why it needs to be checked a third time. And actually, I think that's probably a good indicator that singling out single-step #DB isn't the correct fix, it just happens to be the only case that's been encountered thus far, e.g. a #PF when fetching the instr for emulation should also get priority over the preemption timer. On real hardware, expiration of the preemption timer while vectoring a #PF wouldn't wouldn't get recognized until the next instruction boundary, i.e. at the start of the first instruction of the #PF handler. Dropping the #PF isn't a problem in most cases, because unlike the single-step #DB, it will be re-encountered when L1 resumes L2. But, dropping the #PF is still wrong.In general, interception of an event doesn't change the priority of events, e.g. INTR shouldn't get priority over NMI just because if L1 wants to intercept INTR but not NMI.
TL;DR: I think the fix should instead be:
arch/x86/kvm/vmx/nested.c +12
-6@@ -3724,9 +3724,10 @@ static int vmx_check_nested_events(struct kvm_vcpu *vcpu)/* * Process any exceptions that are not debug traps before MTF. */ -if (vcpu->arch.exception.pending && - !vmx_pending_dbg_trap (vcpu) && -nested_vmx_check_exception (vcpu, &exit_qual)) { +if (vcpu->arch.exception.pending && !vmx_pending_dbg_trap (vcpu)) +if (!nested_vmx_check_exception (vcpu, &exit_qual)) +return 0 ; +if (block_nested_events)return -EBUSY ;nested_vmx_inject_exception_vmexit (vcpu, exit_qual);@@ -3741,8 +3742,10 @@ static int vmx_check_nested_events(struct kvm_vcpu *vcpu)return 0 ; } -if (vcpu->arch.exception.pending && -nested_vmx_check_exception (vcpu, &exit_qual)) { +if (vcpu->arch.exception.pending) { +if (!nested_vmx_check_exception (vcpu, &exit_qual)) +return 0 ; +if (block_nested_events)return -EBUSY ;nested_vmx_inject_exception_vmexit (vcpu, exit_qual);@@ -3757,7 +3760,10 @@ static int vmx_check_nested_events(struct kvm_vcpu *vcpu)return 0 ; } -if (vcpu->arch.nmi_pending &&nested_exit_on_nmi (vcpu)) { +if (vcpu->arch.nmi_pending) { +if (!nested_exit_on_nmi (vcpu)) +return 0 ; +if (block_nested_events)return -EBUSY ; nested_vmx_vmexit(vcpu, EXIT_REASON_EXCEPTION_NMI,
arch/x86/kvm/vmx/nested.c static int vmx_check_nested_events(struct kvm_vcpu *vcpu) · line 3781
Quoted 5 lines
arch/x86/kvm/vmx/nested.c +1
-1@@ -3772,7 +3778,10 @@ static int vmx_check_nested_events(struct kvm_vcpu *vcpu)return 0 ; } -if (kvm_cpu_has_interrupt (vcpu) &&nested_exit_on_intr (vcpu)) { + if (kvm_cpu_has_interrupt(vcpu) {
Obviously untested, because this doesn't compile due to a missing ')'.
Quoted 7 lines
arch/x86/kvm/vmx/nested.c +3
-0+if (!nested_exit_on_intr (vcpu)) +return 0 ; +if (block_nested_events)return -EBUSY ;nested_vmx_vmexit (vcpu,EXIT_REASON_EXTERNAL_INTERRUPT ,0 ,0 );
Re: [PATCH 2/2] kvm: nVMX: Single-step traps trump expired VMX-preemption timer
Quoted 26 lines · Sean Christopherson
Quoted 25 lines · Sean Christopherson
Quoted 5 lines · Jim Mattson
Regarding
-EBUSY, I'm in complete agreement. However, I'm not sure what the potential confusion is regarding the event. Are you suggesting that one might think that we have a #DB to deliver to L1 while we're in guest mode? IIRC, that can happen under SVM, but I don't believe it can happen under VMX.The potential confusion is that
vcpu->arch.exception.pendingwas already checked, twice. It makes one wonder why it needs to be checked a third time. And actually, I think that's probably a good indicator that singling out single-step #DB isn't the correct fix, it just happens to be the only case that's been encountered thus far, e.g. a #PF when fetching the instr for emulation should also get priority over the preemption timer. On real hardware, expiration of the preemption timer while vectoring a #PF wouldn't wouldn't get recognized until the next instruction boundary, i.e. at the start of the first instruction of the #PF handler. Dropping the #PF isn't a problem in most cases, because unlike the single-step #DB, it will be re-encountered when L1 resumes L2. But, dropping the #PF is still wrong.In general, interception of an event doesn't change the priority of events, e.g. INTR shouldn't get priority over NMI just because if L1 wants to intercept INTR but not NMI.
TL;DR: I think the fix should instead be:
Quoted 12 lines
arch/x86/kvm/vmx/nested.c static int vmx_check_nested_events(struct kvm_vcpu *vcpu) · line 3727
Quoted 12 lines
arch/x86/kvm/vmx/nested.c +1
-3@@ -3724,9 +3724,10 @@ static int vmx_check_nested_events(struct kvm_vcpu *vcpu)/* * Process any exceptions that are not debug traps before MTF. */ -if (vcpu->arch.exception.pending && - !vmx_pending_dbg_trap (vcpu) && -nested_vmx_check_exception (vcpu, &exit_qual)) { +if (vcpu->arch.exception.pending && !vmx_pending_dbg_trap (vcpu))
*sigh* And a missing '{' here.
Quoted 47 lines
Quoted 32 lines
arch/x86/kvm/vmx/nested.c +11
-3+if (!nested_vmx_check_exception (vcpu, &exit_qual)) +return 0 ; +if (block_nested_events)return -EBUSY ;nested_vmx_inject_exception_vmexit (vcpu, exit_qual);@@ -3741,8 +3742,10 @@ static int vmx_check_nested_events(struct kvm_vcpu *vcpu)return 0 ; } -if (vcpu->arch.exception.pending && -nested_vmx_check_exception (vcpu, &exit_qual)) { +if (vcpu->arch.exception.pending) { +if (!nested_vmx_check_exception (vcpu, &exit_qual)) +return 0 ; +if (block_nested_events)return -EBUSY ;nested_vmx_inject_exception_vmexit (vcpu, exit_qual);@@ -3757,7 +3760,10 @@ static int vmx_check_nested_events(struct kvm_vcpu *vcpu)return 0 ; } -if (vcpu->arch.nmi_pending &&nested_exit_on_nmi (vcpu)) { +if (vcpu->arch.nmi_pending) { +if (!nested_exit_on_nmi (vcpu)) +return 0 ; +if (block_nested_events)return -EBUSY ; nested_vmx_vmexit(vcpu, EXIT_REASON_EXCEPTION_NMI,arch/x86/kvm/vmx/nested.c static int vmx_check_nested_events(struct kvm_vcpu *vcpu) · line 3781
Quoted 5 lines
arch/x86/kvm/vmx/nested.c +1
-1@@ -3772,7 +3778,10 @@ static int vmx_check_nested_events(struct kvm_vcpu *vcpu)return 0 ; } -if (kvm_cpu_has_interrupt (vcpu) &&nested_exit_on_intr (vcpu)) { + if (kvm_cpu_has_interrupt(vcpu) {Obviously untested, because this doesn't compile due to a missing ')'.
Quoted 7 lines
arch/x86/kvm/vmx/nested.c +3
-0+if (!nested_exit_on_intr (vcpu)) +return 0 ; +if (block_nested_events)return -EBUSY ;nested_vmx_vmexit (vcpu,EXIT_REASON_EXTERNAL_INTERRUPT ,0 ,0 );
Re: [PATCH 2/2] kvm: nVMX: Single-step traps trump expired VMX-preemption timer
In reply to Sean ChristophersonQuoted 44 lines · Sean Christopherson
Quoted 36 lines · Jim Mattson
Quoted 19 lines · Sean Christopherson
Quoted 17 lines · Jim Mattson
Previously, if the hrtimer for the nested VMX-preemption timer fired while L0 was emulating an L2 instruction with RFLAGS.TF set, the synthesized single-step trap would be unceremoniously dropped when synthesizing the "VMX-preemption timer expired" VM-exit from L2 to L1.
To fix this, don't synthesize a "VMX-preemption timer expired" VM-exit from L2 to L1 when there is a pending debug trap, such as a single-step trap.
Fixes f4124500c2c13 ("KVM: nVMX: Fully emulate preemption timer")Signed-off-by Jim Mattson <[email protected]>Reviewed-by Oliver Upton <[email protected]>Reviewed-by Peter Shier <[email protected]>--- arch/x86/kvm/vmx/nested.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-)Quoted 14 lines
arch/x86/kvm/vmx/nested.c static int vmx_check_nested_events(struct kvm_vcpu *vcpu) · line 3693
Quoted 10 lines
arch/x86/kvm/vmx/nested.c +1
-1@@ -3690,7 +3690,9 @@ static int vmx_check_nested_events(struct kvm_vcpu *vcpu) vmx->nested.preemption_timer_expired) {if (block_nested_events)return -EBUSY ; -nested_vmx_vmexit (vcpu,EXIT_REASON_PREEMPTION_TIMER ,0 ,0 ); +if (!vmx_pending_dbg_trap (vcpu))IMO this one warrants a comment. It's not immediately obvious that this only applies to #DBs that are being injected into L2, and that returning
-EBUSYwill do the wrong thing.Quoted 5 lines
Regarding
-EBUSY, I'm in complete agreement. However, I'm not sure what the potential confusion is regarding the event. Are you suggesting that one might think that we have a #DB to deliver to L1 while we're in guest mode? IIRC, that can happen under SVM, but I don't believe it can happen under VMX.
Quoted 11 lines
The potential confusion is that
vcpu->arch.exception.pendingwas already checked, twice. It makes one wonder why it needs to be checked a third time. And actually, I think that's probably a good indicator that singling out single-step #DB isn't the correct fix, it just happens to be the only case that's been encountered thus far, e.g. a #PF when fetching the instr for emulation should also get priority over the preemption timer. On real hardware, expiration of the preemption timer while vectoring a #PF wouldn't wouldn't get recognized until the next instruction boundary, i.e. at the start of the first instruction of the #PF handler. Dropping the #PF isn't a problem in most cases, because unlike the single-step #DB, it will be re-encountered when L1 resumes L2. But, dropping the #PF is still wrong.
Yes, it's wrong in the abstract, but with respect to faults and the VMX-preemption timer expiration, is there any way for either L1 or L2 to *know* that the virtual CPU has done something wrong?
Isn't it generally true that if you have an exception queued when you transition from L2 to L1, then you've done something wrong? I wonder if the call to kvm_clear_exception_queue() in prepare_vmcs12() just serves to sweep a whole collection of problems under the rug.
Quoted 3 lines
In general, interception of an event doesn't change the priority of events, e.g. INTR shouldn't get priority over NMI just because if L1 wants to intercept INTR but not NMI.
Yes, but that's a different problem altogether.
Quoted 58 lines
TL;DR: I think the fix should instead be:
arch/x86/kvm/vmx/nested.c +16
-7@@ -3724,9 +3724,10 @@ static int vmx_check_nested_events(struct kvm_vcpu *vcpu)/* * Process any exceptions that are not debug traps before MTF. */ -if (vcpu->arch.exception.pending && - !vmx_pending_dbg_trap (vcpu) && -nested_vmx_check_exception (vcpu, &exit_qual)) { +if (vcpu->arch.exception.pending && !vmx_pending_dbg_trap (vcpu)) +if (!nested_vmx_check_exception (vcpu, &exit_qual)) +return 0 ; +if (block_nested_events)return -EBUSY ;nested_vmx_inject_exception_vmexit (vcpu, exit_qual);@@ -3741,8 +3742,10 @@ static int vmx_check_nested_events(struct kvm_vcpu *vcpu)return 0 ; } -if (vcpu->arch.exception.pending && -nested_vmx_check_exception (vcpu, &exit_qual)) { +if (vcpu->arch.exception.pending) { +if (!nested_vmx_check_exception (vcpu, &exit_qual)) +return 0 ; +if (block_nested_events)return -EBUSY ;nested_vmx_inject_exception_vmexit (vcpu, exit_qual);@@ -3757,7 +3760,10 @@ static int vmx_check_nested_events(struct kvm_vcpu *vcpu)return 0 ; } -if (vcpu->arch.nmi_pending &&nested_exit_on_nmi (vcpu)) { +if (vcpu->arch.nmi_pending) { +if (!nested_exit_on_nmi (vcpu)) +return 0 ; +if (block_nested_events)return -EBUSY ; nested_vmx_vmexit(vcpu, EXIT_REASON_EXCEPTION_NMI,@@ -3772,7 +3778,10 @@ static int vmx_check_nested_events(struct kvm_vcpu *vcpu)return 0 ; } -if (kvm_cpu_has_interrupt (vcpu) &&nested_exit_on_intr (vcpu)) { + if (kvm_cpu_has_interrupt(vcpu) { +if (!nested_exit_on_intr (vcpu)) +return 0 ; +if (block_nested_events)return -EBUSY ;nested_vmx_vmexit (vcpu,EXIT_REASON_EXTERNAL_INTERRUPT ,0 ,0 );
Re: [PATCH 2/2] kvm: nVMX: Single-step traps trump expired VMX-preemption timer
Quoted 22 lines · Jim Mattson
Quoted 8 lines · Sean Christopherson
Quoted 5 lines · Jim Mattson
Regarding
-EBUSY, I'm in complete agreement. However, I'm not sure what the potential confusion is regarding the event. Are you suggesting that one might think that we have a #DB to deliver to L1 while we're in guest mode? IIRC, that can happen under SVM, but I don't believe it can happen under VMX.Quoted 11 lines
The potential confusion is that
vcpu->arch.exception.pendingwas already checked, twice. It makes one wonder why it needs to be checked a third time. And actually, I think that's probably a good indicator that singling out single-step #DB isn't the correct fix, it just happens to be the only case that's been encountered thus far, e.g. a #PF when fetching the instr for emulation should also get priority over the preemption timer. On real hardware, expiration of the preemption timer while vectoring a #PF wouldn't wouldn't get recognized until the next instruction boundary, i.e. at the start of the first instruction of the #PF handler. Dropping the #PF isn't a problem in most cases, because unlike the single-step #DB, it will be re-encountered when L1 resumes L2. But, dropping the #PF is still wrong.
Quoted 3 lines
Yes, it's wrong in the abstract, but with respect to faults and the VMX-preemption timer expiration, is there any way for either L1 or L2 to *know* that the virtual CPU has done something wrong?
I don't think so? But how is that relevant, i.e. if we can fix KVM instead of fudging the result, why wouldn't we fix KVM?
Quoted 4 lines
Isn't it generally true that if you have an exception queued when you transition from L2 to L1, then you've done something wrong? I wonder if the call to
kvm_clear_exception_queue()inprepare_vmcs12()just serves to sweep a whole collection of problems under the rug.
More than likely, yes.
Quoted 5 lines
Quoted 3 lines
In general, interception of an event doesn't change the priority of events, e.g. INTR shouldn't get priority over NMI just because if L1 wants to intercept INTR but not NMI.
Yes, but that's a different problem altogether.
But isn't the fix the same? Stop processing events if a higher priority event is pending, regardless of whether the event exits to L1.
Quoted 58 lines
Quoted 58 lines
TL;DR: I think the fix should instead be:
arch/x86/kvm/vmx/nested.c +16
-7@@ -3724,9 +3724,10 @@ static int vmx_check_nested_events(struct kvm_vcpu *vcpu)/* * Process any exceptions that are not debug traps before MTF. */ -if (vcpu->arch.exception.pending && - !vmx_pending_dbg_trap (vcpu) && -nested_vmx_check_exception (vcpu, &exit_qual)) { +if (vcpu->arch.exception.pending && !vmx_pending_dbg_trap (vcpu)) +if (!nested_vmx_check_exception (vcpu, &exit_qual)) +return 0 ; +if (block_nested_events)return -EBUSY ;nested_vmx_inject_exception_vmexit (vcpu, exit_qual);@@ -3741,8 +3742,10 @@ static int vmx_check_nested_events(struct kvm_vcpu *vcpu)return 0 ; } -if (vcpu->arch.exception.pending && -nested_vmx_check_exception (vcpu, &exit_qual)) { +if (vcpu->arch.exception.pending) { +if (!nested_vmx_check_exception (vcpu, &exit_qual)) +return 0 ; +if (block_nested_events)return -EBUSY ;nested_vmx_inject_exception_vmexit (vcpu, exit_qual);@@ -3757,7 +3760,10 @@ static int vmx_check_nested_events(struct kvm_vcpu *vcpu)return 0 ; } -if (vcpu->arch.nmi_pending &&nested_exit_on_nmi (vcpu)) { +if (vcpu->arch.nmi_pending) { +if (!nested_exit_on_nmi (vcpu)) +return 0 ; +if (block_nested_events)return -EBUSY ; nested_vmx_vmexit(vcpu, EXIT_REASON_EXCEPTION_NMI,@@ -3772,7 +3778,10 @@ static int vmx_check_nested_events(struct kvm_vcpu *vcpu)return 0 ; } -if (kvm_cpu_has_interrupt (vcpu) &&nested_exit_on_intr (vcpu)) { + if (kvm_cpu_has_interrupt(vcpu) { +if (!nested_exit_on_intr (vcpu)) +return 0 ; +if (block_nested_events)return -EBUSY ;nested_vmx_vmexit (vcpu,EXIT_REASON_EXTERNAL_INTERRUPT ,0 ,0 );
Re: [PATCH 2/2] kvm: nVMX: Single-step traps trump expired VMX-preemption timer
Quoted 24 lines · Sean Christopherson
Quoted 22 lines · Jim Mattson
Quoted 19 lines · Sean Christopherson
Quoted 5 lines · Jim Mattson
Regarding
-EBUSY, I'm in complete agreement. However, I'm not sure what the potential confusion is regarding the event. Are you suggesting that one might think that we have a #DB to deliver to L1 while we're in guest mode? IIRC, that can happen under SVM, but I don't believe it can happen under VMX.The potential confusion is that
vcpu->arch.exception.pendingwas already checked, twice. It makes one wonder why it needs to be checked a third time. And actually, I think that's probably a good indicator that singling out single-step #DB isn't the correct fix, it just happens to be the only case that's been encountered thus far, e.g. a #PF when fetching the instr for emulation should also get priority over the preemption timer. On real hardware, expiration of the preemption timer while vectoring a #PF wouldn't wouldn't get recognized until the next instruction boundary, i.e. at the start of the first instruction of the #PF handler. Dropping the #PF isn't a problem in most cases, because unlike the single-step #DB, it will be re-encountered when L1 resumes L2. But, dropping the #PF is still wrong.
Quoted 6 lines
Quoted 3 lines
Yes, it's wrong in the abstract, but with respect to faults and the VMX-preemption timer expiration, is there any way for either L1 or L2 to *know* that the virtual CPU has done something wrong?
I don't think so? But how is that relevant, i.e. if we can fix KVM instead of fudging the result, why wouldn't we fix KVM?
I'm not sure that I can fix KVM. The missing #DB traps were relatively straightforward, but as for the rest of this mess...
Since you seem to have a handle on what needs to be done, I will defer to you.
Quoted 7 lines
Quoted 4 lines
Isn't it generally true that if you have an exception queued when you transition from L2 to L1, then you've done something wrong? I wonder if the call to
kvm_clear_exception_queue()inprepare_vmcs12()just serves to sweep a whole collection of problems under the rug.More than likely, yes.
Quoted 8 lines
Quoted 5 lines
Quoted 3 lines
In general, interception of an event doesn't change the priority of events, e.g. INTR shouldn't get priority over NMI just because if L1 wants to intercept INTR but not NMI.
Yes, but that's a different problem altogether.
But isn't the fix the same? Stop processing events if a higher priority event is pending, regardless of whether the event exits to L1.
That depends on how you see the scope of the problem. One could argue that the fix for everything that is wrong with KVM is actually the
same: properly emulate the physical CPU.
Re: [PATCH 2/2] kvm: nVMX: Single-step traps trump expired VMX-preemption timer
Quoted 12 lines · Jim Mattson
Quoted 10 lines · Sean Christopherson
Quoted 7 lines · Jim Mattson
Quoted 2 lines · Sean Christopherson
On Tue, Apr 14, 2020 at 09:47:53AM -0700, Jim Mattson wrote:
Yes, it's wrong in the abstract, but with respect to faults and the VMX-preemption timer expiration, is there any way for either L1 or L2 to *know* that the virtual CPU has done something wrong?
Quoted 8 lines
Quoted 2 lines
I don't think so? But how is that relevant, i.e. if we can fix KVM instead of fudging the result, why wouldn't we fix KVM?
I'm not sure that I can fix KVM. The missing #DB traps were relatively straightforward, but as for the rest of this mess...
Since you seem to have a handle on what needs to be done, I will defer to you.
I wouldn't go so far as to say I have a handle on it, more like I have an idea of how to fix one part of the overall problem with a generic "rule" change that also happens to (hopefully) resolve the #DB+MTF issue.
Anyways, I'll send a patch. Worst case scenario it fails miserably and we go with this patch :-)
Quoted 11 lines
Quoted 11 lines
Quoted 4 lines
Isn't it generally true that if you have an exception queued when you transition from L2 to L1, then you've done something wrong? I wonder if the call to
kvm_clear_exception_queue()inprepare_vmcs12()just serves to sweep a whole collection of problems under the rug.More than likely, yes.
Quoted 4 lines
Quoted 3 lines
In general, interception of an event doesn't change the priority of events, e.g. INTR shouldn't get priority over NMI just because if L1 wants to intercept INTR but not NMI.
Quoted 8 lines
Quoted 4 lines
Quoted 1 line
Yes, but that's a different problem altogether.
But isn't the fix the same? Stop processing events if a higher priority event is pending, regardless of whether the event exits to L1.
That depends on how you see the scope of the problem. One could argue that the fix for everything that is wrong with KVM is actually the
same: properly emulate the physical CPU.
Heh, there is that.
What I'm arguing is that we shouldn't throw in a workaround knowing that it's papering over the underlying issue. Preserving event priority irrespective of VM-Exit behavior is different, in that while it may not resolve all issues that are being masked by kvm_clear_exception_queue(), the change itself is correct when viewed in a vacuum.
Re: [PATCH 2/2] kvm: nVMX: Single-step traps trump expired VMX-preemption timer
Quoted 50 lines · Sean Christopherson
Quoted 12 lines · Jim Mattson
Quoted 10 lines · Sean Christopherson
Quoted 7 lines · Jim Mattson
Quoted 2 lines · Sean Christopherson
On Tue, Apr 14, 2020 at 09:47:53AM -0700, Jim Mattson wrote:
Yes, it's wrong in the abstract, but with respect to faults and the VMX-preemption timer expiration, is there any way for either L1 or L2 to *know* that the virtual CPU has done something wrong?
Quoted 8 lines
Quoted 2 lines
I don't think so? But how is that relevant, i.e. if we can fix KVM instead of fudging the result, why wouldn't we fix KVM?
I'm not sure that I can fix KVM. The missing #DB traps were relatively straightforward, but as for the rest of this mess...
Since you seem to have a handle on what needs to be done, I will defer to you.
I wouldn't go so far as to say I have a handle on it, more like I have an idea of how to fix one part of the overall problem with a generic "rule" change that also happens to (hopefully) resolve the #DB+MTF issue.
Anyways, I'll send a patch. Worst case scenario it fails miserably and we go with this patch :-)
Quoted 11 lines
Quoted 11 lines
Quoted 4 lines
Isn't it generally true that if you have an exception queued when you transition from L2 to L1, then you've done something wrong? I wonder if the call to
kvm_clear_exception_queue()inprepare_vmcs12()just serves to sweep a whole collection of problems under the rug.More than likely, yes.
Quoted 4 lines
Quoted 3 lines
In general, interception of an event doesn't change the priority of events, e.g. INTR shouldn't get priority over NMI just because if L1 wants to intercept INTR but not NMI.
Quoted 8 lines
Quoted 4 lines
Quoted 1 line
Yes, but that's a different problem altogether.
But isn't the fix the same? Stop processing events if a higher priority event is pending, regardless of whether the event exits to L1.
That depends on how you see the scope of the problem. One could argue that the fix for everything that is wrong with KVM is actually the
same: properly emulate the physical CPU.
Quoted 7 lines
Heh, there is that.
What I'm arguing is that we shouldn't throw in a workaround knowing that it's papering over the underlying issue. Preserving event priority irrespective of VM-Exit behavior is different, in that while it may not resolve all issues that are being masked by
kvm_clear_exception_queue(), the change itself is correct when viewed in a vacuum.
The more I look at that call to kvm_clear_exception_queue(), the more convinced I am that it's wrong. The comment above it says:
/*
* Drop what we picked up for L2 via vmx_complete_interrupts. It is
* preserved above and would only end up incorrectly in L1. */
The first sentence is just wrong. Vmx_complete_interrupts may not be where the NMI/exception/interrupt came from. And the second sentence is not entirely true. Only *injected* events are "preserved above" (by the call to vmcs12_save_pending_event). However, kvm_clear_exception_queue zaps both injected events and pending events. Moreover, vmcs12_save_pending_event "preserves" the event by stashing it in the IDT-vectoring info field of vmcs12, even when the current VM-exit (from L2 to L1) did not (and in some cases cannot) occur during event delivery (e.g. VMX-preemption timer expired).
Re: [PATCH 2/2] kvm: nVMX: Single-step traps trump expired VMX-preemption timer
Quoted 8 lines · Jim Mattson
The more I look at that call to
kvm_clear_exception_queue(), the more convinced I am that it's wrong. The comment above it says:/*
* Drop what we picked up for L2 viavmx_complete_interrupts. It is
* preserved above and would only end up incorrectly in L1. */
Quoted 9 lines
The first sentence is just wrong.
Vmx_complete_interruptsmay not be where the NMI/exception/interrupt came from. And the second sentence is not entirely true. Only *injected* events are "preserved above" (by the call tovmcs12_save_pending_event). However,kvm_clear_exception_queuezaps both injected events and pending events. Moreover,vmcs12_save_pending_event"preserves" the event by stashing it in the IDT-vectoring info field of vmcs12, even when the current VM-exit (from L2 to L1) did not (and in some cases cannot) occur during event delivery (e.g. VMX-preemption timer expired).
The comments are definitely wrong, or perhaps incomplete, but the behavior isn't "wrong, assuming the rest of the nested event handling is correct (hint, it's not). Even with imperfect event handling, clearing queued/pending exceptions on nested exit isn't terrible behavior as VMX doesn't have a notion of queued/pending excpetions (ignoring #DBs for now). If the VM-Exit occured while vectoring an event, that event is captured in IDT_VECTORING_INFO. Everything else that might have happened is an alternate timeline.
The piece that's likely missing is updating GUEST_PENDING_DBG_EXCEPTIONS if a single-step #DB was pending after emulation. KVM currently propagates the field as-is from vmcs02, which would miss any emulated #DB. But, that is effectively orthogonal to kvm_clear_exception_queue(), e.g. KVM needs to "save" the pending #DB like it "saves" an injected event, but clearing the #DB from the queue is correct.
The main issue I have with the nested event code is that the checks are effectively grouped by exiting versus non-exiting instead of being grouped by priority. That makes it extremely difficult to correctly prioritize events because each half needs to know the other's behavior, even though the code is "separated" E.g. my suggestion to return immediatel if an NMI is pending and won't VM-Exit is wrong, as the behavior should also be conditioned on NMIs being unmasked in L2. Actually implementing that check is a gigantic mess in the current code base and simply isn't worth the marginal benefit.
Unwinding the mess, i.e. processing each event class exactly once per run loop, is extremely difficult because there one-off cases that have been piled on top, e.g. calling check_nested_events() from kvm_vcpu_running() and processing INIT and SIPI in kvm_apic_accept_events(), whose full impact is impossible to ascertain simply by reading the code. The KVM_REQ_EVENT optimization also exacerbates the problem, i.e. the event checking really should be done unconditionally on every loop.
Paolo Bonzini
maintainerRe: [PATCH 2/2] kvm: nVMX: Single-step traps trump expired VMX-preemption timer
In reply to Jim MattsonQuoted 8 lines · Jim Mattson
Previously, if the hrtimer for the nested VMX-preemption timer fired while L0 was emulating an L2 instruction with RFLAGS.TF set, the synthesized single-step trap would be unceremoniously dropped when synthesizing the "VMX-preemption timer expired" VM-exit from L2 to L1.
To fix this, don't synthesize a "VMX-preemption timer expired" VM-exit from L2 to L1 when there is a pending debug trap, such as a single-step trap.
Do you have a testcase for these bugs?
Paolo
Re: [PATCH 2/2] kvm: nVMX: Single-step traps trump expired VMX-preemption timer
Quoted 11 lines · Paolo Bonzini
Quoted 8 lines · Jim Mattson
Previously, if the hrtimer for the nested VMX-preemption timer fired while L0 was emulating an L2 instruction with RFLAGS.TF set, the synthesized single-step trap would be unceremoniously dropped when synthesizing the "VMX-preemption timer expired" VM-exit from L2 to L1.
To fix this, don't synthesize a "VMX-preemption timer expired" VM-exit from L2 to L1 when there is a pending debug trap, such as a single-step trap.
Do you have a testcase for these bugs?
Just in case you're feeling trigger happy, I'm working on a set of patches to fix this in a more generic fashion. Well, fixing this specific issue can be done in a single patch, but NMIs and interrupts technically suffer from the same bug and fixing those requires a bit of extra elbow grease.
There are also (theoretical) bugs related to nested exceptions and interrupt injection that I'm trying to address. Unfortunately I don't have testcases for any of this :-(.
Re: [PATCH 2/2] kvm: nVMX: Single-step traps trump expired VMX-preemption timer
In reply to Paolo BonziniQuoted 7 lines · Paolo Bonzini
Quoted 5 lines · Jim Mattson
Previously, if the hrtimer for the nested VMX-preemption timer fired while L0 was emulating an L2 instruction with RFLAGS.TF set, the synthesized single-step trap would be unceremoniously dropped when synthesizing the "VMX-preemption timer expired" VM-exit from L2 to L1.
Quoted 5 lines
Quoted 3 lines
To fix this, don't synthesize a "VMX-preemption timer expired" VM-exit from L2 to L1 when there is a pending debug trap, such as a single-step trap.
Do you have a testcase for these bugs?
Indeed. They should be just prior to this in your inbox.
Re: [PATCH 2/2] kvm: nVMX: Single-step traps trump expired VMX-preemption timer
Quoted 8 lines · Jim Mattson
Quoted 7 lines · Paolo Bonzini
Quoted 5 lines · Jim Mattson
Previously, if the hrtimer for the nested VMX-preemption timer fired while L0 was emulating an L2 instruction with RFLAGS.TF set, the synthesized single-step trap would be unceremoniously dropped when synthesizing the "VMX-preemption timer expired" VM-exit from L2 to L1.
Quoted 7 lines
Quoted 5 lines
Quoted 3 lines
To fix this, don't synthesize a "VMX-preemption timer expired" VM-exit from L2 to L1 when there is a pending debug trap, such as a single-step trap.
Do you have a testcase for these bugs?
Indeed. They should be just prior to this in your inbox.
Ah, I missed those too and apparently didn't think to search for preemption timer tests. Thanks for the refresher!
Quoted 4 lines · Jim Mattson
Fixes f4124500c2c13 ("KVM: nVMX: Fully emulate preemption timer")Signed-off-by Jim Mattson <[email protected]>Reviewed-by Oliver Upton <[email protected]>Reviewed-by Peter Shier <[email protected]>
...
arch/x86/kvm/vmx/vmx.c static int handle_exception_nmi(struct kvm_vcpu *vcpu) · line 4685
Quoted 10 lines
arch/x86/kvm/vmx/vmx.c +1
-1@@ -4682,7 +4682,7 @@ static int handle_exception_nmi(struct kvm_vcpu *vcpu)if (is_icebp (intr_info))WARN_ON (!skip_emulated_instruction (vcpu)); -kvm_queue_exception (vcpu,DB_VECTOR ); +kvm_requeue_exception (vcpu,DB_VECTOR );
This isn't wrong per se, but it's effectively papering over an underlying bug, e.g. the same missed preemption timer bug can manifest if the timer expires while in KVM context (because the hr timer is left running) and KVM queues an exception for _any_ reason. Most of the scenarios where L0 will queue an exception for L2 are fairly contrived, but they are possible.
I believe the correct fix is to open a "preemption timer window" like we do for pending SMI, NMI and IRQ. It's effectively the same handling a pending SMI on VMX, set req_immediate_exit in the !inject_pending_event() path.
Patches incoming soon-ish, think I've finally got my head wrapped around all the combinations, though I also thought that was true several hours ago...
Quoted 15 lines
arch/x86/kvm/vmx/vmx.c +1
-1return 1 ; } kvm_run->debug.arch.dr6 = dr6 |DR6_FIXED_1 ;@@ -4703,7 +4703,7 @@ static int handle_exception_nmi(struct kvm_vcpu *vcpu)break ;case AC_VECTOR :if (guest_inject_ac (vcpu)) { -kvm_queue_exception_e (vcpu,AC_VECTOR , error_code); +kvm_requeue_exception_e (vcpu,AC_VECTOR , error_code);return 1 ; }Signature · 1 line
2.26.0.110.g2183baf09c-goog
Quoted 16 lines · Sean Christopherson
Quoted 4 lines · Jim Mattson
Fixes f4124500c2c13 ("KVM: nVMX: Fully emulate preemption timer")Signed-off-by Jim Mattson <[email protected]>Reviewed-by Oliver Upton <[email protected]>Reviewed-by Peter Shier <[email protected]>...
Quoted 8 lines
arch/x86/kvm/vmx/vmx.c +0
-0@@ -4682,7 +4682,7 @@ static int handle_exception_nmi(struct kvm_vcpu *vcpu)if (is_icebp (intr_info))WARN_ON (!skip_emulated_instruction (vcpu));
Quoted 7 lines
arch/x86/kvm/vmx/vmx.c static int handle_exception_nmi(struct kvm_vcpu *vcpu) · line 4685
Quoted 2 lines
arch/x86/kvm/vmx/vmx.c +1
-1-kvm_queue_exception (vcpu,DB_VECTOR ); +kvm_requeue_exception (vcpu,DB_VECTOR );This isn't wrong per se, but it's effectively papering over an underlying bug, e.g. the same missed preemption timer bug can manifest if the timer expires while in KVM context (because the hr timer is left running) and KVM queues an exception for _any_ reason.
I just reread your changelog and realized this patch was intended to fix a different symptom than what I observed, i.e. the above probably doesn't make a whole lot of sense. I just so happened that this change also resolved my "missing timer" bug because directly injecting the #DB would cause vmx_check_nested_events() to return -EBUSY on the preemption timer.
That being said, I'm 99% certain that the behavior you observed is fixed by correctly handling priority of non-exiting events vs. exiting events, i.e. slightly different justification, same net result.
Quoted 17 lines · Sean Christopherson
Quoted 4 lines · Jim Mattson
Fixes f4124500c2c13 ("KVM: nVMX: Fully emulate preemption timer")Signed-off-by Jim Mattson <[email protected]>Reviewed-by Oliver Upton <[email protected]>Reviewed-by Peter Shier <[email protected]>...
Quoted 8 lines
arch/x86/kvm/vmx/vmx.c +0
-0@@ -4682,7 +4682,7 @@ static int handle_exception_nmi(struct kvm_vcpu *vcpu)if (is_icebp (intr_info))WARN_ON (!skip_emulated_instruction (vcpu));
Quoted 8 lines
arch/x86/kvm/vmx/vmx.c static int handle_exception_nmi(struct kvm_vcpu *vcpu) · line 4685
Quoted 2 lines
arch/x86/kvm/vmx/vmx.c +1
-1-kvm_queue_exception (vcpu,DB_VECTOR ); +kvm_requeue_exception (vcpu,DB_VECTOR );This isn't wrong per se, but it's effectively papering over an underlying bug, e.g. the same missed preemption timer bug can manifest if the timer expires while in KVM context (because the hr timer is left running) and KVM queues an exception for _any_ reason. Most of the scenarios where L0 will queue an exception for L2 are fairly contrived, but they are possible.
I'm now thinking that this is actually wrong, since requeued exceptions may make their way into the vmcs12 IDT-vectoring info, and I believe that ultimately the vmcs12 IDT-vectoring info should be populated only from the vmcs02 IDT-vectoring info (by whatever circuitous route KVM likes).
Quoted 3 lines
I believe the correct fix is to open a "preemption timer window" like we do for pending SMI, NMI and IRQ. It's effectively the same handling a pending SMI on VMX, set
req_immediate_exitin the !inject_pending_event() path.
The KVM code that deals with all of these events is really hard to follow. I wish we could take a step back and just implement Table 6-2 from the SDM volume 3 (augmented with the scattered information about VMX events and their priorities relative to their nearest neighbors. Lumping priorities 7 - 10 together (faults that we either intercepted or synthesized in emulation), I think these are the various things we need to check, in this order...
0. Is there a fault to be delivered? (In L2, is it intercepted by L1?)
1. Is there a RESET or machine check event?
2. Is there a trap on task switch?
3. Is there an SMI or an INIT?
3.5 In L2, is there an MTF VM-exit?
4. Is there a #DB trap on the previous instruction? (In L2, is it intercepted by L1?)
4.3 In L2, has the VMX-preemption timer expired?
4.6 In L2, do we need to synthesize an NMI-window VM-exit?
5. Is there an NMI? (In L2, is it intercepted by L1?)
5.3 In L2 do we need to synthesize an interrupt-window VM-exit? 5.6 In L2, do we need to virtualize virtual-interrupt delivery?
6. Is there a maskable interrupt? (In L2, is it intercepted by L1?)
7. Now, we can enter VMX non-root mode.
Quoted 10 lines · Jim Mattson
On Wed, Apr 22, 2020 at 2:06 PM Sean Christopherson <[email protected]> wrote:
The KVM code that deals with all of these events is really hard to follow. I wish we could take a step back and just implement Table 6-2 from the SDM volume 3 (augmented with the scattered information about VMX events and their priorities relative to their nearest neighbors. Lumping priorities 7 - 10 together (faults that we either intercepted or synthesized in emulation), I think these are the various things we need to check, in this order...
Quoted 14 lines
0. Is there a fault to be delivered? (In L2, is it intercepted by L1?)
1. Is there a RESET or machine check event?
2. Is there a trap on task switch?
3. Is there an SMI or an INIT?
3.5 In L2, is there an MTF VM-exit?
4. Is there a #DB trap on the previous instruction? (In L2, is it intercepted by L1?)
4.3 In L2, has the VMX-preemption timer expired?
4.6 In L2, do we need to synthesize an NMI-window VM-exit?
5. Is there an NMI? (In L2, is it intercepted by L1?)
5.3 In L2 do we need to synthesize an interrupt-window VM-exit? 5.6 In L2, do we need to virtualize virtual-interrupt delivery?
6. Is there a maskable interrupt? (In L2, is it intercepted by L1?)
7. Now, we can enter VMX non-root mode.
100% agreed. I even tried to go down that path, multiple times, while sorting this stuff out. The big problem that isn't easily resolved is kvm_vcpu_running(), which currently calls .check_nested_events() even if KVM_REQ_EVENT isn't set. Its existence makes it annoyingly difficult to provide a unified single-pass flow for exiting and non-exiting events, e.g. we'd either have to duplicate a big pile of logic (eww) or significantly rework the event handling (scary).
Having the INIT and SIPI handling buried in kvm_apic_accept_events() is also a pain, but that's less scary to change.
In the long term, I absolutely think it'd be worth revamping the event handling so that it's not scattered all over tarnation, but that's something that should probably have a full kernel cycle or two of testing and performance analysis.
If someone does pick up that torch, I think it'd also be worth experimenting with removing KVM_REQ_EVENT, i.e. processing events on _every_ run. IMO that would simplify the code, or at least how one reasons about the code, a great deal.