KVM: VMX: Update instruction length on intercepted BP
PATCH v2 1/1 KVM: VMX: Update instruction length on intercepted BP
We intercept #BP while in guest debugging mode. As VM exits due to intercepted exceptions do not necessarily come with valid idt_vectoring, we have to update event_exit_inst_len explicitly in such cases. At least in the absence of migration, this ensures that re-injections of #BP will find and use the correct instruction length.
---
Changes in v2:
- added comments
arch/x86/kvm/vmx.c | 13 +++++++++++++ 1 files changed, 13 insertions(+), 0 deletions(-)
arch/x86/kvm/vmx.c +13 -0
@@ -2775,6 +2775,12 @@ static int handle_rmode_exception(struct kvm_vcpu *vcpu,kvm_queue_exception (vcpu, vec);return 1 ;case BP_VECTOR : +/* +* Update instruction length as we may reinject the exception +* from user space while in guest debugging mode. +*/ +to_vmx (vcpu)->vcpu.arch.event_exit_inst_len = +vmcs_read32 (VM_EXIT_INSTRUCTION_LEN );if (vcpu->guest_debug &KVM_GUESTDBG_USE_SW_BP )return 0 ;/* fall through */
@@ -2897,6 +2903,13 @@ static int handle_exception(struct kvm_vcpu *vcpu) kvm_run->debug.arch.dr7 =vmcs_readl (GUEST_DR7 );/* fall through */ case BP_VECTOR : +/* +* Update instruction length as we may reinject #BP from +* user space while in guest debugging mode. Reading it for +* #DB as well causes no harm, it is not used in that case. +*/ + vmx->vcpu.arch.event_exit_inst_len = +vmcs_read32 (VM_EXIT_INSTRUCTION_LEN ); kvm_run->exit_reason =KVM_EXIT_DEBUG ; kvm_run->debug.arch.pc =vmcs_readl (GUEST_CS_BASE ) + rip; kvm_run->debug.arch.exception = ex_no;
Quoted 6 lines · Jan Kiszka
We intercept #BP while in guest debugging mode. As VM exits due to intercepted exceptions do not necessarily come with valid
idt_vectoring, we have to updateevent_exit_inst_lenexplicitly in such cases. At least in the absence of migration, this ensures that re-injections of #BP will find and use the correct instruction length.
Thinking about it some more. Why do we exit to userspace at all if we intercept wrong #DB? It seams to me not wise to have ability to inject exceptions from userspace. Exceptions generation mechanism is a part of CPU and we shouldn't outsource part of CPU functionality to userspace.
Quoted 30 lines
Signed-off-by Jan Kiszka <[email protected]>---
Changes in v2:
- added commentsarch/x86/kvm/vmx.c | 13 +++++++++++++ 1 files changed, 13 insertions(+), 0 deletions(-)arch/x86/kvm/vmx.c +6
-0@@ -2775,6 +2775,12 @@ static int handle_rmode_exception(struct kvm_vcpu *vcpu,kvm_queue_exception (vcpu, vec);return 1 ;case BP_VECTOR : +/* +* Update instruction length as we may reinject the exception +* from user space while in guest debugging mode. +*/ +to_vmx (vcpu)->vcpu.arch.event_exit_inst_len = +vmcs_read32 (VM_EXIT_INSTRUCTION_LEN );if (vcpu->guest_debug &KVM_GUESTDBG_USE_SW_BP )return 0 ;/* fall through */ @@ -2897,6 +2903,13 @@ static int handle_exception(struct kvm_vcpu *vcpu) kvm_run->debug.arch.dr7 =vmcs_readl (GUEST_DR7 );/* fall through */ case BP_VECTOR :
arch/x86/kvm/vmx.c static int handle_exception(struct kvm_vcpu *vcpu) · line 2915
Quoted 10 lines
arch/x86/kvm/vmx.c +7
-0+/* +* Update instruction length as we may reinject #BP from +* user space while in guest debugging mode. Reading it for +* #DB as well causes no harm, it is not used in that case. +*/ + vmx->vcpu.arch.event_exit_inst_len = +vmcs_read32 (VM_EXIT_INSTRUCTION_LEN ); kvm_run->exit_reason =KVM_EXIT_DEBUG ; kvm_run->debug.arch.pc =vmcs_readl (GUEST_CS_BASE ) + rip; kvm_run->debug.arch.exception = ex_no;
--
Gleb.
Quoted 11 lines · Gleb Natapov
Quoted 6 lines · Jan Kiszka
We intercept #BP while in guest debugging mode. As VM exits due to intercepted exceptions do not necessarily come with valid
idt_vectoring, we have to updateevent_exit_inst_lenexplicitly in such cases. At least in the absence of migration, this ensures that re-injections of #BP will find and use the correct instruction length.Thinking about it some more. Why do we exit to userspace at all if we intercept wrong #DB? It seams to me not wise to have ability to inject exceptions from userspace. Exceptions generation mechanism is a part of CPU and we shouldn't outsource part of CPU functionality to userspace.
The guest debugging API was design to avoid maintaining a "countless" number of breakpoints in kernel space and instead chose to loop over user space to decide about #DB & #BP. So this part is required even if we start thinking about an alternative interface in the future.
Jan
Quoted 44 lines
Quoted 31 lines
Signed-off-by Jan Kiszka <[email protected]>---
Changes in v2:
- added commentsarch/x86/kvm/vmx.c | 13 +++++++++++++ 1 files changed, 13 insertions(+), 0 deletions(-)arch/x86/kvm/vmx.c +6
-0@@ -2775,6 +2775,12 @@ static int handle_rmode_exception(struct kvm_vcpu *vcpu,kvm_queue_exception (vcpu, vec);return 1 ;case BP_VECTOR : +/* +* Update instruction length as we may reinject the exception +* from user space while in guest debugging mode. +*/ +to_vmx (vcpu)->vcpu.arch.event_exit_inst_len = +vmcs_read32 (VM_EXIT_INSTRUCTION_LEN );if (vcpu->guest_debug &KVM_GUESTDBG_USE_SW_BP )return 0 ;/* fall through */ @@ -2897,6 +2903,13 @@ static int handle_exception(struct kvm_vcpu *vcpu) kvm_run->debug.arch.dr7 =vmcs_readl (GUEST_DR7 );/* fall through */ case BP_VECTOR :arch/x86/kvm/vmx.c static int handle_exception(struct kvm_vcpu *vcpu) · line 2915
Quoted 10 lines
arch/x86/kvm/vmx.c +7
-0+/* +* Update instruction length as we may reinject #BP from +* user space while in guest debugging mode. Reading it for +* #DB as well causes no harm, it is not used in that case. +*/ + vmx->vcpu.arch.event_exit_inst_len = +vmcs_read32 (VM_EXIT_INSTRUCTION_LEN ); kvm_run->exit_reason =KVM_EXIT_DEBUG ; kvm_run->debug.arch.pc =vmcs_readl (GUEST_CS_BASE ) + rip; kvm_run->debug.arch.exception = ex_no;--
Gleb.
Quoted 13 lines · Jan Kiszka
Quoted 11 lines · Gleb Natapov
Quoted 6 lines · Jan Kiszka
We intercept #BP while in guest debugging mode. As VM exits due to intercepted exceptions do not necessarily come with valid
idt_vectoring, we have to updateevent_exit_inst_lenexplicitly in such cases. At least in the absence of migration, this ensures that re-injections of #BP will find and use the correct instruction length.Thinking about it some more. Why do we exit to userspace at all if we intercept wrong #DB? It seams to me not wise to have ability to inject exceptions from userspace. Exceptions generation mechanism is a part of CPU and we shouldn't outsource part of CPU functionality to userspace.
Quoted 5 lines
The guest debugging API was design to avoid maintaining a "countless" number of breakpoints in kernel space and instead chose to loop over user space to decide about #DB & #BP. So this part is required even if we start thinking about an alternative interface in the future.
How much is "countless"? 10000? I am sure we can handle this.
Quoted 47 lines
Jan
Quoted 44 lines
Quoted 31 lines
Signed-off-by Jan Kiszka <[email protected]>---
Changes in v2:
- added commentsarch/x86/kvm/vmx.c | 13 +++++++++++++ 1 files changed, 13 insertions(+), 0 deletions(-)arch/x86/kvm/vmx.c +6
-0@@ -2775,6 +2775,12 @@ static int handle_rmode_exception(struct kvm_vcpu *vcpu,kvm_queue_exception (vcpu, vec);return 1 ;case BP_VECTOR : +/* +* Update instruction length as we may reinject the exception +* from user space while in guest debugging mode. +*/ +to_vmx (vcpu)->vcpu.arch.event_exit_inst_len = +vmcs_read32 (VM_EXIT_INSTRUCTION_LEN );if (vcpu->guest_debug &KVM_GUESTDBG_USE_SW_BP )return 0 ;/* fall through */ @@ -2897,6 +2903,13 @@ static int handle_exception(struct kvm_vcpu *vcpu) kvm_run->debug.arch.dr7 =vmcs_readl (GUEST_DR7 );/* fall through */ case BP_VECTOR :arch/x86/kvm/vmx.c static int handle_exception(struct kvm_vcpu *vcpu) · line 2915
Quoted 10 lines
arch/x86/kvm/vmx.c +7
-0+/* +* Update instruction length as we may reinject #BP from +* user space while in guest debugging mode. Reading it for +* #DB as well causes no harm, it is not used in that case. +*/ + vmx->vcpu.arch.event_exit_inst_len = +vmcs_read32 (VM_EXIT_INSTRUCTION_LEN ); kvm_run->exit_reason =KVM_EXIT_DEBUG ; kvm_run->debug.arch.pc =vmcs_readl (GUEST_CS_BASE ) + rip; kvm_run->debug.arch.exception = ex_no;--
Gleb.
--
Gleb.
Quoted 13 lines · Gleb Natapov
Quoted 12 lines · Jan Kiszka
Quoted 11 lines · Gleb Natapov
Quoted 6 lines · Jan Kiszka
We intercept #BP while in guest debugging mode. As VM exits due to intercepted exceptions do not necessarily come with valid
idt_vectoring, we have to updateevent_exit_inst_lenexplicitly in such cases. At least in the absence of migration, this ensures that re-injections of #BP will find and use the correct instruction length.Thinking about it some more. Why do we exit to userspace at all if we intercept wrong #DB? It seams to me not wise to have ability to inject exceptions from userspace. Exceptions generation mechanism is a part of CPU and we shouldn't outsource part of CPU functionality to userspace.
Quoted 6 lines
Quoted 5 lines
The guest debugging API was design to avoid maintaining a "countless" number of breakpoints in kernel space and instead chose to loop over user space to decide about #DB & #BP. So this part is required even if we start thinking about an alternative interface in the future.
How much is "countless"? 10000? I am sure we can handle this.
We could even handle more. But would have to
- handle INT3 injection in kernel space, including step-over on resume
- fully parse HW breakpoints in kernel space
- probably deal with some more complications that are now handled in user space, part of them even in gdb
And, again: This is an _existing_ user space ABI. We could only provide an alternative, but we have to maintain what is there at least for some longer grace period.
Jan
Signature · 2 lines
Siemens AG, Corporate Technology, CT T DE IT 1 Corporate Competence Center Embedded Linux
Quoted 19 lines · Jan Kiszka
Quoted 18 lines · Gleb Natapov
Quoted 17 lines · Jan Kiszka
Quoted 11 lines · Gleb Natapov
Quoted 6 lines · Jan Kiszka
We intercept #BP while in guest debugging mode. As VM exits due to intercepted exceptions do not necessarily come with valid
idt_vectoring, we have to updateevent_exit_inst_lenexplicitly in such cases. At least in the absence of migration, this ensures that re-injections of #BP will find and use the correct instruction length.Thinking about it some more. Why do we exit to userspace at all if we intercept wrong #DB? It seams to me not wise to have ability to inject exceptions from userspace. Exceptions generation mechanism is a part of CPU and we shouldn't outsource part of CPU functionality to userspace.
The guest debugging API was design to avoid maintaining a "countless" number of breakpoints in kernel space and instead chose to loop over user space to decide about #DB & #BP. So this part is required even if we start thinking about an alternative interface in the future.
Quoted 8 lines
Quoted 1 line
How much is "countless"? 10000? I am sure we can handle this.
We could even handle more. But would have to
- handle INT3 injection in kernel space, including step-over on resume
- fully parse HW breakpoints in kernel space
- probably deal with some more complications that are now handled in user space, part of them even in gdb
The first point in this list is needed no anyway, no matter who reinjects #BP event. About point three what are those complications? As far as I see all we need to know in kernel is a list of cr3:address pairs that have breakpoint set. If #BP intercept happens we scan this list and if match is not found reinject event to the guest otherwise exit to userspace.
Quoted 4 lines
And, again: This is an _existing_ user space ABI. We could only provide an alternative, but we have to maintain what is there at least for some longer grace period.
But it was always broken for SVM and was broken for VMX for a year and nobody noticed, so may be instead of reintroducing old interface we should do it right this time?
--
Gleb.
Quoted 9 lines · Gleb Natapov
Quoted 5 lines
And, again: This is an _existing_ user space ABI. We could only provide an alternative, but we have to maintain what is there at least for some longer grace period.
But it was always broken for SVM and was broken for VMX for a year and nobody noticed, so may be instead of reintroducing old interface we should do it right this time?
We need to fix the existing interface first, and then think long and hard if we want yet another interface, since we're likely to screw it up as well.
The more interfaces we introduce, the harder maintenance becomes.
Signature · 1 line
error compiling committee.c: too many arguments to function
Quoted 9 lines · Avi Kivity
Quoted 7 lines · Gleb Natapov
Quoted 4 lines
And, again: This is an _existing_ user space ABI. We could only provide an alternative, but we have to maintain what is there at least for some longer grace period.
But it was always broken for SVM and was broken for VMX for a year and nobody noticed, so may be instead of reintroducing old interface we should do it right this time?
Quoted 6 lines
We need to fix the existing interface first, and then think long and hard if we want yet another interface, since we're likely to screw it up as well.
The more interfaces we introduce, the harder maintenance becomes.
We are in a sad state if we cannot improve interface. The current one outsource part of CPU functionality into userspace. This should be a big no-no.
--
Gleb.
Quoted 13 lines · Gleb Natapov
Quoted 12 lines · Avi Kivity
Quoted 7 lines · Gleb Natapov
Quoted 4 lines
And, again: This is an _existing_ user space ABI. We could only provide an alternative, but we have to maintain what is there at least for some longer grace period.
But it was always broken for SVM and was broken for VMX for a year and nobody noticed, so may be instead of reintroducing old interface we should do it right this time?
We need to fix the existing interface first, and then think long and hard if we want yet another interface, since we're likely to screw it up as well.
Quoted 5 lines
Quoted 2 lines
The more interfaces we introduce, the harder maintenance becomes.
We are in a sad state if we cannot improve interface. The current one outsource part of CPU functionality into userspace. This should be a big no-no.
I still disagree on this. Moving the decision logic to user space prevented to re-implement a gdbstub in kernel space. I oversaw that re-injecting #BP over older SVM was broken, but it is now fixed for all vendors. So moving it back to kernel has actually no long-term reason.
Jan
Signature · 2 lines
Siemens AG, Corporate Technology, CT T DE IT 1 Corporate Competence Center Embedded Linux
Quoted 16 lines · Jan Kiszka
Quoted 15 lines · Gleb Natapov
Quoted 14 lines · Avi Kivity
Quoted 7 lines · Gleb Natapov
Quoted 4 lines
And, again: This is an _existing_ user space ABI. We could only provide an alternative, but we have to maintain what is there at least for some longer grace period.
But it was always broken for SVM and was broken for VMX for a year and nobody noticed, so may be instead of reintroducing old interface we should do it right this time?
We need to fix the existing interface first, and then think long and hard if we want yet another interface, since we're likely to screw it up as well.
The more interfaces we introduce, the harder maintenance becomes.
Quoted 9 lines
Quoted 3 lines
We are in a sad state if we cannot improve interface. The current one outsource part of CPU functionality into userspace. This should be a big no-no.
I still disagree on this. Moving the decision logic to user space prevented to re-implement a gdbstub in kernel space. I oversaw that re-injecting #BP over older SVM was broken, but it is now fixed for all vendors. So moving it back to kernel has actually no long-term reason.
There were patches to implement gdbstub in kernel space! And not so long time ago :) But I want to move only a tiny bit of logic into the kernel space. And #BP reinjection brokenness is a different issue. It should be fixed anyway no matter where decision about reinfection happens.
If maintainers think that we should not have improved interface and we should support reinjection of #DB from userspace then this patch should be applied. I don't have other objections to it. But I, at least, would prefer the old interface for #DB reinjection (KVM_GUESTDBG_INJECT_DB) and not the new one. The old one makes it explicit what we are doing, the new one allows injection of any event and should be used only during migration or CPU reset. It would be event good idea to fail setting events if CPU is running.
--
Gleb.
Quoted 20 lines · Gleb Natapov
Quoted 19 lines · Jan Kiszka
Quoted 18 lines · Gleb Natapov
Quoted 5 lines · Avi Kivity
Quoted 4 lines · Gleb Natapov
Quoted 4 lines
And, again: This is an _existing_ user space ABI. We could only provide an alternative, but we have to maintain what is there at least for some longer grace period.
Quoted 9 lines
Quoted 3 lines
But it was always broken for SVM and was broken for VMX for a year and nobody noticed, so may be instead of reintroducing old interface we should do it right this time?
We need to fix the existing interface first, and then think long and hard if we want yet another interface, since we're likely to screw it up as well.
The more interfaces we introduce, the harder maintenance becomes.
We are in a sad state if we cannot improve interface. The current one outsource part of CPU functionality into userspace. This should be a big no-no.
Quoted 7 lines
Quoted 5 lines
I still disagree on this. Moving the decision logic to user space prevented to re-implement a gdbstub in kernel space. I oversaw that re-injecting #BP over older SVM was broken, but it is now fixed for all vendors. So moving it back to kernel has actually no long-term reason.
There were patches to implement gdbstub in kernel space! And not so long time ago :)
Yes, a good reason to implement yet another one. :)
Quoted 4 lines
But I want to move only a tiny bit of logic into the kernel space. And #BP reinjection brokenness is a different issue. It should be fixed anyway no matter where decision about reinfection happens.
Quoted 8 lines
If maintainers think that we should not have improved interface and we should support reinjection of #DB from userspace then this patch should be applied. I don't have other objections to it. But I, at least, would prefer the old interface for #DB reinjection (
KVM_GUESTDBG_INJECT_DB) and not the new one. The old one makes it explicit what we are doing, the new one allows injection of any event and should be used only during migration or CPU reset. It would be event good idea to fail setting events if CPU is running.
Event injection is well supported by both vendors (except for those software-triggered events). Just because QEMU mostly uses it for reset and migration doesn't mean we have to restrict other users to only those cases as well.
And as we have true event injection now, and as it naturally conflicts with the special KVM_SET_GUEST_DEBUG interface, I have a patch that consolidates this usage for QEMU: use the old interface of SET_GUEST_DEBUG for pre-2.6.33 kernels, switch to SET_VCPU_EVENTS on recent ones.
Jan
Signature · 2 lines
Siemens AG, Corporate Technology, CT T DE IT 1 Corporate Competence Center Embedded Linux
Quoted 26 lines · Jan Kiszka
Quoted 25 lines · Gleb Natapov
Quoted 24 lines · Jan Kiszka
Quoted 13 lines · Gleb Natapov
Quoted 12 lines · Avi Kivity
Quoted 7 lines · Gleb Natapov
Quoted 4 lines
And, again: This is an _existing_ user space ABI. We could only provide an alternative, but we have to maintain what is there at least for some longer grace period.
But it was always broken for SVM and was broken for VMX for a year and nobody noticed, so may be instead of reintroducing old interface we should do it right this time?
We need to fix the existing interface first, and then think long and hard if we want yet another interface, since we're likely to screw it up as well.
Quoted 5 lines
Quoted 2 lines
The more interfaces we introduce, the harder maintenance becomes.
We are in a sad state if we cannot improve interface. The current one outsource part of CPU functionality into userspace. This should be a big no-no.
I still disagree on this. Moving the decision logic to user space prevented to re-implement a gdbstub in kernel space. I oversaw that re-injecting #BP over older SVM was broken, but it is now fixed for all vendors. So moving it back to kernel has actually no long-term reason.
Quoted 5 lines
Quoted 2 lines
There were patches to implement gdbstub in kernel space! And not so long time ago :)
Yes, a good reason to implement yet another one. :)
We can you unify them later :). But seriously I am not proposing anything like gdbstub in kernel, just track inserted breakpoints in kernel.
Quoted 13 lines
Quoted 4 lines
But I want to move only a tiny bit of logic into the kernel space. And #BP reinjection brokenness is a different issue. It should be fixed anyway no matter where decision about reinfection happens.
Quoted 8 lines
If maintainers think that we should not have improved interface and we should support reinjection of #DB from userspace then this patch should be applied. I don't have other objections to it. But I, at least, would prefer the old interface for #DB reinjection (
KVM_GUESTDBG_INJECT_DB) and not the new one. The old one makes it explicit what we are doing, the new one allows injection of any event and should be used only during migration or CPU reset. It would be event good idea to fail setting events if CPU is running.
Quoted 4 lines
Event injection is well supported by both vendors (except for those software-triggered events). Just because QEMU mostly uses it for reset and migration doesn't mean we have to restrict other users to only those cases as well.
Yes we have too! Qemu implements device model and the way devices communicates with CPU is well defined and called interrupts, so we have a way to inject interrupts (KVM_IRQ_LINE/KVM_INTERRUPT). Input is validated and passed into VCPU in the right time, we do not inject interrupts directly into VCPU using event injection. Exceptions, on the other hand, is completely internal CPU thing. QEMU shouldn't be a part of CPU emulation.
Quoted 2 lines
And as we have true event injection now, and as it naturally conflicts
Now we have a bug that should be fixed ASAP. We should allow setting of some VCPU state only when VCPU is stopped and only for migration/reset purposes.
Quoted 4 lines
with the special
KVM_SET_GUEST_DEBUGinterface, I have a patch that consolidates this usage for QEMU: use the old interface ofSET_GUEST_DEBUGfor pre-2.6.33 kernels, switch toSET_VCPU_EVENTSon recent ones.
Don't do that please, this will encourage use of SET_VCPU_EVENTS for something it shouldn't be used for.
--
Gleb.