| Message ID | 20161128041856.11420-6-khuey@kylehuey.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 28/11/2016 05:18, Kyle Huey wrote: > + > + if (unlikely(vmx_get_rflags(vcpu) & X86_EFLAGS_TF)) { > + if (vcpu->guest_debug & KVM_GUESTDBG_SINGLESTEP) { > + vcpu->run->debug.arch.dr6 = DR6_BS | DR6_FIXED_1 | > + DR6_RTM; > + vcpu->run->debug.arch.pc = vcpu->arch.singlestep_rip; > + vcpu->run->debug.arch.exception = DB_VECTOR; > + vcpu->run->exit_reason = KVM_EXIT_DEBUG; > + return 0; > + } > + > + /* > + * "Certain debug exceptions may clear bit 0-3. The > + * remaining contents of the DR6 register are never > + * cleared by the processor". > + */ > + vcpu->arch.dr6 &= ~15; > + vcpu->arch.dr6 |= DR6_BS | DR6_RTM; > + kvm_queue_exception(vcpu, DB_VECTOR); > + } This code is pretty much the same as kvm_vcpu_check_singlestep. Let's not duplicate the code and implement skip_emulated_instruction can be implemented in x86.c, like unsigned long rflags = kvm_x86_ops->get_rflags(vcpu); int r = EMULATE_DONE; /* This would be the no_trap variant */ kvm_x86_ops->skip_emulated_instruction(vcpu); kvm_vcpu_check_singlestep(vcpu, rflags, &r); return r == EMULATE_DONE; (because x86.c/vmx.c/svm.c are separate modules, when moving the function to x86.c you should rename it to kvm_skip_emulated_instruction). Paolo -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, Nov 28, 2016 at 3:42 AM, Paolo Bonzini <pbonzini@redhat.com> wrote: > > > On 28/11/2016 05:18, Kyle Huey wrote: >> + >> + if (unlikely(vmx_get_rflags(vcpu) & X86_EFLAGS_TF)) { >> + if (vcpu->guest_debug & KVM_GUESTDBG_SINGLESTEP) { >> + vcpu->run->debug.arch.dr6 = DR6_BS | DR6_FIXED_1 | >> + DR6_RTM; >> + vcpu->run->debug.arch.pc = vcpu->arch.singlestep_rip; >> + vcpu->run->debug.arch.exception = DB_VECTOR; >> + vcpu->run->exit_reason = KVM_EXIT_DEBUG; >> + return 0; >> + } >> + >> + /* >> + * "Certain debug exceptions may clear bit 0-3. The >> + * remaining contents of the DR6 register are never >> + * cleared by the processor". >> + */ >> + vcpu->arch.dr6 &= ~15; >> + vcpu->arch.dr6 |= DR6_BS | DR6_RTM; >> + kvm_queue_exception(vcpu, DB_VECTOR); >> + } > > This code is pretty much the same as kvm_vcpu_check_singlestep. Let's > not duplicate the code and implement skip_emulated_instruction can be > implemented in x86.c, like > > unsigned long rflags = kvm_x86_ops->get_rflags(vcpu); > int r = EMULATE_DONE; > > /* This would be the no_trap variant */ > kvm_x86_ops->skip_emulated_instruction(vcpu); > kvm_vcpu_check_singlestep(vcpu, rflags, &r); > return r == EMULATE_DONE; > > (because x86.c/vmx.c/svm.c are separate modules, when moving the function > to x86.c you should rename it to kvm_skip_emulated_instruction). > > Paolo They're not exactly the same. For some reason I don't understand kvm_vcpu_check_singlestep clears the trap flag. Perhaps that is also a bug? - Kyle -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 28/11/2016 17:13, Kyle Huey wrote: > On Mon, Nov 28, 2016 at 3:42 AM, Paolo Bonzini <pbonzini@redhat.com> wrote: >> This code is pretty much the same as kvm_vcpu_check_singlestep. Let's >> not duplicate the code and implement skip_emulated_instruction can be >> implemented in x86.c, like >> >> unsigned long rflags = kvm_x86_ops->get_rflags(vcpu); >> int r = EMULATE_DONE; >> >> /* This would be the no_trap variant */ >> kvm_x86_ops->skip_emulated_instruction(vcpu); >> kvm_vcpu_check_singlestep(vcpu, rflags, &r); >> return r == EMULATE_DONE; >> >> (because x86.c/vmx.c/svm.c are separate modules, when moving the function >> to x86.c you should rename it to kvm_skip_emulated_instruction). >> >> Paolo > > They're not exactly the same. For some reason I don't understand > kvm_vcpu_check_singlestep clears the trap flag. Perhaps that is also > a bug? The Intel manual says "The processor clears the TF flag before calling the exception handler" (17.3.1.4), so I think you should do it too. Paolo -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, Nov 28, 2016 at 9:19 AM, Paolo Bonzini <pbonzini@redhat.com> wrote: > On 28/11/2016 17:13, Kyle Huey wrote: >> On Mon, Nov 28, 2016 at 3:42 AM, Paolo Bonzini <pbonzini@redhat.com> wrote: >>> This code is pretty much the same as kvm_vcpu_check_singlestep. Let's >>> not duplicate the code and implement skip_emulated_instruction can be >>> implemented in x86.c, like >>> >>> unsigned long rflags = kvm_x86_ops->get_rflags(vcpu); >>> int r = EMULATE_DONE; >>> >>> /* This would be the no_trap variant */ >>> kvm_x86_ops->skip_emulated_instruction(vcpu); >>> kvm_vcpu_check_singlestep(vcpu, rflags, &r); >>> return r == EMULATE_DONE; >>> >>> (because x86.c/vmx.c/svm.c are separate modules, when moving the function >>> to x86.c you should rename it to kvm_skip_emulated_instruction). >>> >>> Paolo >> >> They're not exactly the same. For some reason I don't understand >> kvm_vcpu_check_singlestep clears the trap flag. Perhaps that is also >> a bug? > > The Intel manual says "The processor clears the TF flag before calling > the exception handler" (17.3.1.4), so I think you should do it too. The processor does this automatically. "When accessing an exception or interrupt handler through either an interrupt gate or a trap gate, the processor clears the TF flag in the EFLAGS register after it saves the contents of the EFLAGS register on the stack." (Vol 3, 6.12.1.2) Empirically, this holds when injecting an exception on VM entry. If you take the x86/debug.c test from kvm-unit-tests and inspect RFLAGS in handle_db (not regs->rflags, but the actual RFLAGS register while running the exception handler) the TF is clear. And, if you modify my patch to clear TF before returning, the single stepping ceases after the CPUID instruction because the TF was in fact cleared for good. - Kyle -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 28/11/2016 19:34, Kyle Huey wrote: >> > The Intel manual says "The processor clears the TF flag before calling >> > the exception handler" (17.3.1.4), so I think you should do it too. > The processor does this automatically. "When accessing an exception or > interrupt handler through either an interrupt gate or a trap gate, the > processor clears the TF flag in the EFLAGS register after it saves the > contents of the EFLAGS register on the stack." (Vol 3, 6.12.1.2) > Empirically, this holds when injecting an exception on VM entry. If > you take the x86/debug.c test from kvm-unit-tests and inspect RFLAGS > in handle_db (not regs->rflags, but the actual RFLAGS register while > running the exception handler) the TF is clear. And, if you modify my > patch to clear TF before returning, the single stepping ceases after > the CPUID instruction because the TF was in fact cleared for good. Ok, then that would be a bug in kvm_vcpu_check_singlestep (because kvm_vcpu_check_singlestep is mostly interesting for real mode emulation, I checked kvm_inject_realmode_interrupt and it clears TF too, in __emulate_int_real). Paolo -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c index f404aef..6583e97 100644 --- a/arch/x86/kvm/vmx.c +++ b/arch/x86/kvm/vmx.c @@ -2470,16 +2470,37 @@ static void skip_emulated_instruction_no_trap(struct kvm_vcpu *vcpu) /* skipping an emulated instruction also counts */ vmx_set_interrupt_shadow(vcpu, 0); } static int skip_emulated_instruction(struct kvm_vcpu *vcpu) { skip_emulated_instruction_no_trap(vcpu); + + if (unlikely(vmx_get_rflags(vcpu) & X86_EFLAGS_TF)) { + if (vcpu->guest_debug & KVM_GUESTDBG_SINGLESTEP) { + vcpu->run->debug.arch.dr6 = DR6_BS | DR6_FIXED_1 | + DR6_RTM; + vcpu->run->debug.arch.pc = vcpu->arch.singlestep_rip; + vcpu->run->debug.arch.exception = DB_VECTOR; + vcpu->run->exit_reason = KVM_EXIT_DEBUG; + return 0; + } + + /* + * "Certain debug exceptions may clear bit 0-3. The + * remaining contents of the DR6 register are never + * cleared by the processor". + */ + vcpu->arch.dr6 &= ~15; + vcpu->arch.dr6 |= DR6_BS | DR6_RTM; + kvm_queue_exception(vcpu, DB_VECTOR); + } + return 1; } /* * KVM wants to inject page-faults which it got to the guest. This function * checks whether in a nested guest, we need to inject them to L1 or L2. */ static int nested_vmx_check_exception(struct kvm_vcpu *vcpu, unsigned nr)
Similar to the code in kvm_vcpu_check_singlestep, check for TF and, depending on the origin, synthesize a DB exception or an exit to userspace. Signed-off-by: Kyle Huey <khuey@kylehuey.com> --- arch/x86/kvm/vmx.c | 21 +++++++++++++++++++++ 1 file changed, 21 insertions(+)